Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix resolving relative paths with corral run #216

Merged
merged 10 commits into from
Feb 13, 2022
Merged

Conversation

mfelsche
Copy link
Contributor

The logic to determine the absolute path to an executable was using FilePath.from(base, subdir) which errors if the resulting path is not below the base. This is the case if subdir is e.g. ../foo.

This fix stems from trying to execute:

$ corral run -- ../../ponyc/build/debug/ponyc
run: couldn't run program: ../../ponyc/build/debug/ponyc .

The error output is clearly not really informative.
With this PR the above command actually runs the existing ponyc.

Other changes in here all related to improving corral run:

  • First try to resolve relative paths against the current working directory, then against $PATH. This enables running relative paths to parent folders (See the above command).
  • Display different errors if no args were provided or the executable to run could not be found

…ent folders

Signed-off-by: Matthias Wahl <matthiaswahl@m7w3.de>
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Feb 12, 2022
@SeanTAllen
Copy link
Member

So the path you gave was valid but it wasn't run? I'm confused by the explanation.

@mfelsche
Copy link
Contributor Author

mfelsche commented Feb 12, 2022

The path was valid and existing and runnable. The code to resolve relative paths had a flaw in that it used FilePath.from which errors when one tries to escape that path upwards using ... This is how FilePath ensures safety wrt to its capability. In this very case this safety net is actually hurting, so i switched to using Path.join instead.

@SeanTAllen
Copy link
Member

Gotcha

@SeanTAllen
Copy link
Member

SeanTAllen commented Feb 13, 2022

Would you agree this is two changes? If yes, then please add one manually to change log and add two release notes entries. Then add a correct changelog label to this PR and rename the PR to match whatever the automatically added changelog entry should be.

We need a bot added changelog as without a changelog label, release notes are ignored.

@mfelsche mfelsche added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Feb 13, 2022
@mfelsche mfelsche changed the title Improve errors in 'corral run' and allow to run relative paths to parent folders Fix resolving relative paths with corral run Feb 13, 2022
@SeanTAllen
Copy link
Member

@mfelsche this needs a changelog entry manually added for the Improved Error messages for corral run`` item. Can you do that then will be ready for merging. The changelog entry will be added for "Fix resolving relative paths with corral run" automatically

corral/test/util.pony Outdated Show resolved Hide resolved
Matthias Wahl and others added 6 commits February 13, 2022 19:46
ye'ole backslashes...
Co-authored-by: Sean T Allen <sean@seantallen.com>
Co-authored-by: Sean T Allen <sean@seantallen.com>
Co-authored-by: Sean T Allen <sean@seantallen.com>
@SeanTAllen SeanTAllen merged commit 628720b into main Feb 13, 2022
@SeanTAllen SeanTAllen deleted the corral-run-fixes branch February 13, 2022 20:00
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Feb 13, 2022
github-actions bot pushed a commit that referenced this pull request Feb 13, 2022
github-actions bot pushed a commit that referenced this pull request Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants