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 to #1079 #1080

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

fix to #1079 #1080

wants to merge 1 commit into from

Conversation

Julianiolo
Copy link

This isn't the exact fix described in the issue, but I figured it would make more sense to test if we are using bash?

@sbc100
Copy link
Collaborator

sbc100 commented Jul 26, 2022

Interesting.. it looks like we already have special case in place for when we actually construct the path:

emsdk/emsdk.py

Lines 2652 to 2653 in 5ad3ff0

separator = ':' if MSYS else ENVPATH_SEPARATOR
return (separator.join(whole_path), new_emsdk_tools)

We are running the CI here with windows python running under bash.exe.. and it seems that python.exe (the windows version) is sees an incoming path that has windows separators.

@Julianiolo
Copy link
Author

Julianiolo commented Jul 26, 2022

Oh Interesting, yeah then maybe a special check like that should be used instead here:

emsdk/emsdk.py

Line 2623 in 5ad3ff0

existing_path = os.environ['PATH'].split(ENVPATH_SEPARATOR)

That means that this line currently shouln't work for msys though, right?

@Julianiolo
Copy link
Author

Another idea I had was to just try both or check to see if we find a ':' or ';' in the PATH string, but I guess that may be not that robust.

@sbc100
Copy link
Collaborator

sbc100 commented Jul 26, 2022

Another idea I had was to just try both or check to see if we find a ':' or ';' in the PATH string, but I guess that may be not that robust.

I wonder if just os.name == 'nt' here? This seems to suggest that would work because cynwin pyhton reports os.name as posix: https://stackoverflow.com/a/7637706

Can you confirm if that works here?

@sbc100
Copy link
Collaborator

sbc100 commented Jul 26, 2022

Another idea I had was to just try both or check to see if we find a ':' or ';' in the PATH string, but I guess that may be not that robust.

I wonder if just os.name == 'nt' here? This seems to suggest that would work because cynwin pyhton reports os.name as posix: https://stackoverflow.com/a/7637706

Can you confirm if that works here?

i.e. what does os.name and os.pathsep return for your cygwin version of python?

@sbc100
Copy link
Collaborator

sbc100 commented Jul 26, 2022

Another idea I had was to just try both or check to see if we find a ':' or ';' in the PATH string, but I guess that may be not that robust.

I wonder if just os.name == 'nt' here? This seems to suggest that would work because cynwin pyhton reports os.name as posix: https://stackoverflow.com/a/7637706
Can you confirm if that works here?

i.e. what does os.name and os.pathsep return for your cygwin version of python?

Maybe we don't need ENVPATH_SEPARATOR at all and we can just use os.pathsep?

@sbc100
Copy link
Collaborator

sbc100 commented Jul 26, 2022

Another idea I had was to just try both or check to see if we find a ':' or ';' in the PATH string, but I guess that may be not that robust.

I wonder if just os.name == 'nt' here? This seems to suggest that would work because cynwin pyhton reports os.name as posix: https://stackoverflow.com/a/7637706
Can you confirm if that works here?

i.e. what does os.name and os.pathsep return for your cygwin version of python?

Also, what does os.environ['PATH'] report in your version of python?

@Julianiolo
Copy link
Author

Sorry that I took so long to answer ':)

as to your ideas, I think that might be the way to go.

the python related stuff you asked:

$ python
Python 3.9.10 (main, Jan 20 2022, 21:37:52) 
[GCC 11.2.0] on cygwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.name
'posix'
>>> os.pathsep
':'
>>> os.environ['PATH']
'/usr/local/bin:/usr/bin:/cygdrive/d/Programme/General/CUDA/dev/bin:/cygdrive/d/Programme/General/CUDA/dev/libnvvp:/cygdrive/c/Program Files/Common Files/Oracle/Java/javapath:/cygdrive/c/Program Files 
(x86)/Common Files/Oracle/Java/javapath:/cygdrive/c/Windows/system32:/cygdrive/c/Windows:/cygdrive/c/Windows/System32/Wbem:/cygdrive/c/Windows/System32/WindowsPowerShell/v1.0:/cygdrive/c/Windows/System32/OpenSSH:/cygdrive/c/Program Files (x86)/GtkSharp/2.12/bin:/cygdrive/c/WINDOWS/system32:/cygdrive/c/WINDOWS:/cygdrive/c/WINDOWS/System32/Wbem:/cygdrive/c/WINDOWS/System32/WindowsPowerShell/v1.0:/cygdrive/c/WINDOWS/System32/OpenSSH:/cygdrive/c/Program Files (x86)/NVIDIA Corporation/PhysX/Common:/cygdrive/c/Program Files/NVIDIA Corporation/NVIDIA NvDLISR:/cygdrive/c/Program Files/PuTTY:/cygdrive/c/Program Files/nodejs:/cygdrive/c/cmake-3.19.0-rc2-win64-x64/bin:/cygdrive/c/Git/cmd:/usr/bin:/cygdrive/c/Program Files/dotnet:/cygdrive/c/Program Files/Liquid Technologies/Liquid Studio 2021/XmlDataBinder19/Redist19/cpp/win32/bin:/cygdrive/c/Program Files/Liquid Technologies/Liquid Studio 2021/XmlDataBinder19/Redist19/cpp/win64/bin:/cygdrive/d/Programme/General/UltraEdit:/cygdrive/c/Users/Julian/Desktop/Dateien/programms/ffmpeg-2021-10-07-git-b6aeee2d8b-full_build/bin:/cygdrive/c/WINDOWS/system32:/cygdrive/c/WINDOWS:/cygdrive/c/WINDOWS/System32/Wbem:/cygdrive/c/WINDOWS/System32/WindowsPowerShell/v1.0:/cygdrive/c/WINDOWS/System32/OpenSSH:/cygdrive/c/Program Files/NVIDIA Corporation/Nsight Compute 2022.1.1:/cygdrive/d/Programme/General/Graphviz/bin:/cygdrive/c/Users/Julian/Desktop/Dateien/programms/ffmpeg-2022-06-01-git-c6364b711b-essentials_build/bin:/cygdrive/c/Users/Julian/Desktop/Dateien/programms/emsdk:/cygdrive/c/Users/Julian/Desktop/Dateien/programms/emsdk/upstream/emscripten:/cygdrive/c/Users/Julian/Desktop/Dateien/programms/emsdk/node/14.18.2_64bit/bin:/cygdrive/c/Users/Julian/AppData/Local/Programs/Python/Python38/Scripts:/cygdrive/c/Users/Julian/AppData/Local/Programs/Python/Python38:/cygdrive/c/Users/Julian/AppData/Local/Microsoft/WindowsApps:/cygdrive/c/Users/Julian/AppData/Local/atom/bin:/cygdrive/c/Users/Julian/AppData/Local/Programs/Microsoft VS Code/bin:/cygdrive/c/Users/Julian/AppData/Local/GitHubDesktop/bin:/cygdrive/c/Users/Julian/AppData/Roaming/npm:%USERPROFILE%/AppData/Local/Microsoft/WindowsApps'
>>>

so yeah I guess that should work :)

@Julianiolo
Copy link
Author

I didn't know os.pathsep was a thing, but if it exists, maybe we should just use it?

@sbc100
Copy link
Collaborator

sbc100 commented Jul 28, 2022

I didn't know os.pathsep was a thing, but if it exists, maybe we should just use it?

One problem is that sometimes folks use windows python inside a mingw shell. This means os.pathsep will be ; for windows but create_env needs to build a path that is suitable for the mingw shell.

So, we have two different path setups

  1. The paths uses in the incoming os.environ['PATH'. This depends I think on whether we are using windows python of cygwin python. Here we always want to use os.pathsep.
  2. The path convention used by the calling shell. i.e. are we using cmd.exe or bash.exe to run create_env. This could be different to the os.pathsep used by python.exe.

@Julianiolo
Copy link
Author

What do you mean by create_env exactly?

@sbc100
Copy link
Collaborator

sbc100 commented Jul 28, 2022

Sorry I meant construct_env. When you run emsdk_env it called emsdk.py construct_env to build a set of environment variables for your shell.

@Julianiolo
Copy link
Author

what bash do you mean in the context of mingw, the one from git? Or does mingw has its own bash? I'm not too well versed with mingw ':)

@Julianiolo
Copy link
Author

if thats the bash you mean, then yeah there is defenitly a problem as python says 'nt' for name and pathsep is ';' so that wont work for constructing a path, since this bash uses ':'.

Currently I would think that maybe we should do the parsing of the current path with os.pathsep and then for constructing we still need to somehow find the right seperator.

@Julianiolo
Copy link
Author

one idea I have is to maybe just set an environment variable in the activate.bat file and then check for that in python? The bat file should be the only case where a ';' seperated path is needed, right?

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