-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
vsenv: fix ValueError parsing multi-line env variables #13682
base: master
Are you sure you want to change the base?
Conversation
3ed46c2
to
a8c2b2b
Compare
Did you try it again with this patch to meson? What was the reported "illegal variable name"? |
I found the root issue... So the CI adds an env variable
And well... that is broken into lines |
c8d9d18
to
d0241ce
Compare
d0241ce
to
8944331
Compare
I updated the unit tests with 2 env variables that replicates the 2 possible errors:
|
I wonder if it's instead possible to filter those out from |
That is an optimization I can implement. I will also change the code to call the script directly with subprocess instead of having to create a temporary file, that will speed up things as well. |
Some CI's include variables containing the issue description, like GitLab's CI_MERGE_REQUEST_DESCRIPTION. The description itself can have multiple lines and any kind of content. If one of these lines is '=========', it will leave k empty when it's split by '='S. Also improve how lines without '=' are parsed by checking the parts of the split by '=' rather than trying to unpack a 1 value array and handle the exception
794eb52
to
d83e2a9
Compare
Use a command instead of creating a temporary file for the .bat script and process only newly created variables
d83e2a9
to
2a55d29
Compare
I had been thinking to do something like newenv = {k:v for k, v in os.environ.items() if '\n' not in v}
bat_output = subprocess.check_output(bat_file.name, env=newenv, universal_newlines=True,
encoding=locale.getpreferredencoding(False)) Or if we knew the minimal set of existing variables the I suppose iterating over the set difference of lines would work too as long as none of the different lines are themselves part of a multiline variable which I suppose vcvars should not be producing. It just felt a bit weird to be recovering via random sort. But maybe it's fine. |
The test errors appear to be relevant but I do not understand them. |
The error messages come from trying to use the GCC compiler. So maybe |
Somehow the environment is not being set correctly after my changes. I will iterate again over the code and reproduce the error locally with the tests. |
This helps debugging issues like https://gitlab.freedesktop.org/ylatuya/gstreamer/-/jobs/63636384#L259.
Another question is if it should be treated as an error or ignored like it's done when parsing the key-value string a few lines above.