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

Feature/1269 formalize paths in package commands #1273

Merged
merged 10 commits into from
Apr 19, 2022

Conversation

nerdvegas
Copy link
Contributor

Fixes #1269

@nerdvegas nerdvegas linked an issue Apr 5, 2022 that may be closed by this pull request
@JeanChristopheMorinPerso
Copy link
Member

I didn't put a lot of thought into it, but I am tempted to say we could apply such normalization only when required. So in this case, only for the yet to be created GitBash shell plugin. I feel like it would be a much safer approach than applying normalization on all shell types.

I would also make this an opt-in feature (so no default configuration).

@nerdvegas
Copy link
Contributor Author

I didn't put a lot of thought into it, but I am tempted to say we could apply such normalization only when required.

Well, this is what effectively happens anyway. The default impl leaves paths unchanged. The alternative would simply be checking a bool flag to see if normalisation should be done (the code responsible for applying normalization isn't isolated to each shell). So it'd end up as basically the same thing.

I would also make this an opt-in feature (so no default configuration).

I'm not so sure about that, currently windows paths are getting set to (eg) C:\foo\pkg\1.0.0/bin, which isn't great. However, perhaps(?) a setting to disable it could be in order, if it proves necessary.

Note also - this turned out to be a lot more complex than I expected, and there are more changes I need to add to this PR, so I'll probably decline and reopen it. Path normalization is more of a new feature than you'd think - currently there's the implicit assumption that paths are just system native, and not affected by the shell. You'll see all the interesting bits in the new PR, stay tuned.

@nerdvegas
Copy link
Contributor Author

Ok, changes applied, including addition of new wiki entry on filepaths.

To clarify my previous comment: Currently, things are working in Windows only coincidentally (in the sense that it happens to tolerate fwd slashes in filepaths). The unspoken rule so far is that paths in package commands are expected to be posix-style. This goes hand-in-hand with the expectation that env-var references are also unix-like (see https://github.com/nerdvegas/rez/wiki/Package-Commands#environment-variable-expansion).

However, the fact that posix paths are expected, and that all the existing shells actually work, is basically sheer luck. The addition of the new gitbash shell (PR coming) doesn't represent an edge case - it's just the the first shell that happens not to work, because of something we didn't take into account (the need for path normalization, in order to have one common path syntax, which allows us to write non-os-specific pkg commands).

Basically path normalization is required in order to have pkg commands that aren't inadvertently os/platform- dependent. Fortunately, I'm pretty sure this change is backwards compatible with existing commands that do use platform-specific syntax. Specifically, use of os.path.join avoids any issues, as does using backslashes on windows.

I would definitely highlight this info in the release notes though.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 7, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
3.3% 3.3% Duplication

@nerdvegas nerdvegas merged commit b406166 into master Apr 19, 2022
@JeanChristopheMorinPerso
Copy link
Member

I missed your last two comments. I am still against this though. I strongly believe that path normalization should not be applied to all shell types.

I also think it's not too late to make this an opt-in instead of an opt-out and only enforce that when absolutely necessary (so far only for gitbash).

@nerdvegas
Copy link
Contributor Author

nerdvegas commented Apr 20, 2022 via email

@bpabel bpabel deleted the feature/1269-formalize-paths-in-package-commands branch January 19, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

formalize paths in package commands
2 participants