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

On windows, construct environment separately when storing to registry #648

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

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 30, 2020

We can't use the same set of environment variables when setting local
values and when storing to the windows registry.

This is because the new PATH value is constructed very differently
when updating the local shell compared to when updating the
registry.

Add test that verifies that no path elements are removed by
emsdk_env. This includes not re-ordering or removing duplicates
that might exist already in the user's path.

Fixes: #634

@sbc100 sbc100 force-pushed the seperate_path_env branch 2 times, most recently from a4ef38f to 2d6d136 Compare October 30, 2020 16:33
@sbc100 sbc100 force-pushed the seperate_path_env branch 2 times, most recently from 3c12628 to 16faaa3 Compare October 30, 2020 16:39
Copy link
Contributor

@aminya aminya left a comment

Choose a reason for hiding this comment

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

See #642 (comment). That PR considers all the cases (--system, --permamnet, none of them). It includes the tests for all of them.

Copy link
Contributor

@aminya aminya left a comment

Choose a reason for hiding this comment

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

I am interested to know if this fixes the Process PATH preservation.

You can add process test by pasting this code at here and here

        if (Compare-Object -ReferenceObject $PATH_Process_BEFORE.Split(';') -DifferenceObject $PATH_Process.Split(';') ) {
            echo "Old process path is ...................."
            echo $PATH_Process_BEFORE
            echo "Current process path is ................"
            echo $PATH_Process
            throw "User PATH are changed while --system had been provided"
        }

@sbc100 sbc100 force-pushed the seperate_path_env branch 3 times, most recently from 9ad7455 to edd6175 Compare November 10, 2020 06:28
We can't use the set of environment variable when setting local
valus and when storing to the registry.

This is because the new PATH value is constructed very differently
when updating the local shell compared to when updating the
registry.

Add test that verifies that no path elements are removed by
emsdk_env.  This includes not re-ordering or removing duplicates
that might exist already in the user's path.

Fixes: #634
@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 26, 2020

I am interested to know if this fixes the Process PATH preservation.

You can add process test by pasting this code at here and here

        if (Compare-Object -ReferenceObject $PATH_Process_BEFORE.Split(';') -DifferenceObject $PATH_Process.Split(';') ) {
            echo "Old process path is ...................."
            echo $PATH_Process_BEFORE
            echo "Current process path is ................"
            echo $PATH_Process
            throw "User PATH are changed while --system had been provided"
        }

I don't think that will work because activation is supposed to add to the process path so it will be different afterwards.

The test that I've added added in this RR in scripts/check_path.py instread checked that all of the element of the original path still appear in the new path and that none were removed. i.e. that the operation only added to the process path and did not remove or re-order the existing process path.

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.

On Windows emsdk_env.bat removes most of the PATH environment variable
2 participants