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

Improve PowerShell plugin's profile handling functionality #1777

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nrusch
Copy link
Contributor

@nrusch nrusch commented Jun 21, 2024

Improvements to the PowerShell plugin to support profile-related settings/options.

Add support for the package_commands_sourced_first config/runtime option

Prior to this change, the rez PowerShell implementation would always source the rez context script after the user/host profile scripts, so profile-level modifications to environment variables like PATH were always squashed by the unconditional overrides in the context script.

With this change, the relative source order of the shell profile vs. the context script can be properly controlled using the package_commands_sourced_first config option, matching the behavior of SH-based shells.

NOTE: Because this option defaults to True, this also implicitly changes rez's default behavior to source the shell profile after the context when using PowerShell.

Support the norc shell plugin option.

This enables the use of rez-env --norc to bypass the sourcing of any profile scripts when using PowerShell.

@nrusch nrusch requested a review from a team as a code owner June 21, 2024 00:34
Copy link

linux-foundation-easycla bot commented Jun 21, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: nrusch / name: Nathan Rusch (7f3678a)

* Add support for the `package_commands_sourced_first` config switch.

Prior to this change, the rez PowerShell implementation would always
source the rez context script after the user/host profile scripts, so
profile-level modifications to environment variables like `PATH` were
always squashed by the unconditional overrides in the context script.

With this change, the relative source order of the shell profile vs.
the context script can be properly controlled using the
`package_commands_sourced_first` config option, matching the behavior
of `SH`-based shells.

NOTE: Because this config option defaults to True, this commit also
implicitly changes rez's default behavior to source the shell profile
*after* the context when using PowerShell.

* Support the `norc` shell plugin option, which enables the use of
`rez-env --norc` to bypass the sourcing of any profile scripts.

Signed-off-by: Nathan Rusch <[email protected]>
@nrusch nrusch force-pushed the improve_pwsh_profile_handling branch from 5b7157b to 7f3678a Compare June 21, 2024 00:35
Comment on lines +31 to +33
'$PROFILE '
'| Get-Member -type NoteProperty '
'| ForEach-Object { if (Test-Path $PROFILE."$($_.Name)") { . $PROFILE."$($_.Name)" }}')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this is a dynamic approach to discovering available profile scripts, but it may be preferable to hardcode iteration over the known profile slots documented here.

This could look something like this (+/- formatting):

foreach ($name in 'AllUsersAllHosts', 'AllUsersCurrentHost', 'CurrentUserAllHosts', 'CurrentUserCurrentHost')
{
    if (Test-Path $PROFILE.$name) {
        . $PROFILE.$name
    }
}

@nrusch
Copy link
Contributor Author

nrusch commented Jun 21, 2024

This may indirectly "resolve" #1279, since the user's profile will have the final say on any shell customization.

@JeanChristopheMorinPerso JeanChristopheMorinPerso added the Blocked by CLA Waiting on CLA to be signed label Jun 22, 2024
@nrusch
Copy link
Contributor Author

nrusch commented Jul 1, 2024

The CLA has been sorted for rez. I'm not sure why this PR's status isn't updating though...

@JeanChristopheMorinPerso
Copy link
Member

/easycla

@JeanChristopheMorinPerso
Copy link
Member

@nrusch it should be good now. I manually triggered a refresh and it's now green.

@JeanChristopheMorinPerso JeanChristopheMorinPerso removed the Blocked by CLA Waiting on CLA to be signed label Jul 1, 2024
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.

2 participants