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

Override CWD in runEngineDistribution to the parent of the project being run to avoid warning #10928

Merged

Conversation

radeusgd
Copy link
Member

Pull Request Description

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.

@radeusgd radeusgd added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 30, 2024
@radeusgd radeusgd self-assigned this Aug 30, 2024
Comment on lines +294 to +301
// Override the working directory of new process to be the parent of the project directory.
cwdOverride.foreach { c =>
pb.directory(c)
}
if (cwdOverride.isDefined) {
// If the working directory is changed, we need to translate the path - make it absolute.
all.set(runArgumentIndex.get + 1, runArgumentAsFile.get.getAbsolutePath)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is that because the CWD changes, if there were any other paths given as arguments, they may now become invalid.

But I don't think we often add paths to runEngineDistribution, do we? Maybe I should add a comment to be sure to prefer absolute paths?

I was thinking of a 'heuristic' that will convent relative paths to absolute, but it would be too imprecise - is a test argument a path to the ./test/ directory or a completely different thing? Impossible to tell without context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only path I really care about is after --run.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for a quick fix. Too bad it only works for runEngineDistribution and not generally from CLI. What kind of language Enso is when it cannot execute files referenced by a path?

Try(new File(path)).toOption

/** Looks for a parent directory that contains `package.yaml`. */
private def findProjectRoot(file: File): Option[File] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

@radeusgd
Copy link
Member Author

Thanks for a quick fix. Too bad it only works for runEngineDistribution and not generally from CLI. What kind of language Enso is when it cannot execute files referenced by a path?

It can - it's just a warning. We warn the user that to ensure consistency with the GUI behaviour, they should run the engine with correctly set working directory. Using ensoup sets that up automatically

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Aug 31, 2024
@mergify mergify bot merged commit 240ac1a into develop Aug 31, 2024
38 checks passed
@mergify mergify bot deleted the wip/radeusgd/avoid-warning-in-runenginedistribution branch August 31, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants