env.unset method

[bruchar1]

My usecase is that I'm running from an active Python venv, and I need to perform some function calls inside different python venv, but the VIRTUAL_ENV environment variable interferes. Therefore, I need to unset it in the environment I use to perform the function call.

[bruchar1]

Is there any existing tests for environment objects?

[dcbaker]

I'd really like to see a unittest for this (the new "testcase" clause should be pretty helpful).

I'm wondering what happens in this case:

env = EnvironmentVariables()
env.set('foo', 'bar')
env.unset('foo')
env.set('foo', 'foo')

Which I would expect to be set foo="foo", but looking at your implementation I think would make foo unset.

I would expect either for foo="foo" or for an error to be raised about trying to modify an env variable that is unset.

[bruchar1]

I'd really like to see a unittest for this (the new "testcase" clause should be pretty helpful).

Not sure how I could use testcase here, since we cannot introspect env vars inside meson.build. Anyway, I managed to write some basic tests.

I'm wondering what happens in this case:

env = EnvironmentVariables()
env.set('foo', 'bar')
env.unset('foo')
env.set('foo', 'foo')

Which I would expect to be set foo="foo", but looking at your implementation I think would make foo unset.

I would expect either for foo="foo" or for an error to be raised about trying to modify an env variable that is unset.

I think my implementation is ok. Whether you modify a variable with set, append, or prepend, it removes it from the unset_vars.

[dcbaker]

Dang, I didn't realize that they couldn't be inspected like a dict or configuration_data. This looks like a good solution though, thanks!

[eli-schwartz]

I think my implementation is ok. Whether you modify a variable with set, append, or prepend, it removes it from the unset_vars.

I agree with the original concern that this seems weird.

In fact, it generally seems weird, though I could see someone wanting to set a variable and then append additional contents to it. But I don't see any use case for unsetting a variable and then appending to it causing meson to revert to the original value of the variable plus whatever you append.

It feels very much like mutually contradictory high-level goals.

[bruchar1]

In fact, it generally seems weird, though I could see someone wanting to set a variable and then append additional contents to it. But I don't see any use case for unsetting a variable and then appending to it causing meson to revert to the original value of the variable plus whatever you append.

I changed that usecase for an error.

set remains possible, as I imagine it could happen in complex situations that a variable could be set or unset depending on some conditions.

[bruchar1]

rebased, and retargeted for 1.4.0

[bruchar1]

@eli-schwartz Was there anything missing? I think I addressed your comments.

[bruchar1]

@eli-schwartz @dcbaker I think all concerns were addressed. Is there anything missing for merging this?

[jpakkane]

In my mind setting and unsetting the same variable is a logical error or at least problematic. It is confusing to have env.set('foo', 'bar') and then find out later that your test fails because somewhere else someone did an .unset.

I'd personally make this a hard error, but other people might have different opinions.

[eli-schwartz]

I'd personally make this a hard error, but other people might have different opinions.

Well, it's already a hard error since the function doesn't exist. :)

Adding the ability to unset a variable does indeed result in the confusing situation you pointed out.

I noted above that it "feels weird" to me.

[eli-schwartz]

@eli-schwartz @dcbaker I think all concerns were addressed. Is there anything missing for merging this?

I appreciate you addressing my concerns. Well, most of them. ;)

I still feel somewhat uncomfortable about the whole feature. If others believe this is worth having I don't feel so uncomfortable about it that I'm going to stand in their way if they want to merge it, but I'd rather not make the decision to merge it myself.

The important thing to me is that you've addressed my concerns about:

Even if we add this feature, I don't believe it should allow re-setting a variable that has been unset

Which as you said above has been addressed.

[bruchar1]

@jpakkane I modified it to prevent setting an unset variable, and vice-versa.

[rgommers]

This PR broke SciPy's dev builds on Windows. It appears that:

• using meson compile -C builddir is always fine
• using ninja -C builddir is fine on non-Windows platforms, but breaks on Windows

A custom_target call in the SciPy build ends with (from scipy/scipy#20433):

AttributeError: 'EnvironmentVariables' object has no attribute 'unset_vars'

I think, but am not 100% sure, that this is a regression. meson-python is not affected, because it uses meson compile on Windows, rather than invoking ninja directly like on other platforms. However, that was implemented for being able to use vsenv auto-detection only IIRC; ninja is expected to work too I believe.

Please let me know if you consider this a regression, and I can open a new bug report with more details and hopefully a clean/simple reproducer.

[eli-schwartz]

self.unset_vars is created inside __init__() along with all other attributes of this class. It's not obvious to me why it should ever produce an AttributeError unless ninja is running the wrong "meson" "--internal" "exe" "--unpickle" executable.

[rgommers]

Agreed, it's not obvious to me either.

unless ninja is running the wrong "meson" "--internal" "exe" "--unpickle" executable.

Looks like it. This is the only command that has is described with (wrapped$ by$ meson$ to$ set$ env) and starts with env. On Linux:

COMMAND = env PYTHONPATH=/Users/rgommers/code/bldscipy/scipy/_build_utils /Users/rgommers/mambaforge/envs/scipy-dev-newcompilers/bin/python3.11 ../scipy/linalg/_generate_pyx.py -o scipy/linalg
 description = Generating$ scipy/linalg/cython_linalg$ with$ a$ custom$ command$ (wrapped$ by$ meson$ to$ set$ env)

I don't actually see how env is defined, it doesn't seem present in build.ninja. For COMMAND = env, where does ninja fetch env from?

[eli-schwartz]

On Linux, /usr/bin/env is a system program that accepts a list of key=value pairs of environment variables followed by a command to run, and sets the environment variables in the temporary environment used to execute that command.

Windows doesn't support POSIX and therefore doesn't have env, so we implement it in Python as part of the "--internal exe" handling.

[rgommers]

Ah of course, thank you. It seems like the original bug report isn't cleanly reproducible after a git clean -xdf (scipy/scipy#20433 (comment)), so it may be something like doing:

1. pip install meson==1.3.0
2. meson setup build
3. pip install meson==1.4.0
4. ninja -C build

that's needed to trigger this. Or somehow mixing two build environments that ended up picking up the wrong meson executable.

[eli-schwartz]

Right, we don't support that. Only bugfix releases are binary-compatible. However often we can detect this and bail out, whereas the exe pickler uses a very minimal set of imports to ensure that builds are fast.

[rgommers]

That last sentence was very helpful context, thanks @eli-schwartz. Now I understand why we never hit this kind of issue before - we only recently started using env = {PYTHONPATH...} for a single build target. Removing that env = usage (done in scipy/scipy#20450) should make things more robust.