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

Meson build helper: make sure env variables from conan profile take precedence #6000

Merged
merged 12 commits into from Feb 3, 2020

Conversation

agurtovoy
Copy link
Contributor

@agurtovoy agurtovoy commented Oct 29, 2019

Changelog: Feature: Adds vcvars_append variable (defaulting to False) to CMake and Meson build helpers constructors, so when they need to activate the Visual Studio environment via vcvars (for Ninja and NMake generators), the vcvars environment is appended at the end, giving precedence to the environment previously defined.
Docs: conan-io/docs#1543

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Oct 29, 2019

CLA assistant check
All committers have signed the CLA.

@memsharded
Copy link
Member

@memsharded memsharded commented Oct 29, 2019

Hi @agurtovoy

Thanks for your contribution. I have reviewed it and I think we need first to understand the scope of this issue, and if it affects other build systems. From what I am seeing in the code, it seems that CMake is not automatically adding the environment as your PR does. And we need to keep the behavior of build helpers consistent.

@memsharded memsharded added this to the 1.21 milestone Oct 29, 2019
@memsharded memsharded removed this from the 1.21 milestone Oct 29, 2019
@agurtovoy
Copy link
Contributor Author

@agurtovoy agurtovoy commented Oct 30, 2019

@memsharded Good point. I should note that, in general, the profile env vars are already properly set by the time a build helper gets control; the issue is that some build helpers invoke tools.vcvars after that, effectively invalidating any prior env setup, specifically as it's concerned with PATH.

@lasote
Copy link
Contributor

@lasote lasote commented Jan 2, 2020

I understand the issue but we cannot solve it applying the conanfile.env at that point. As Diego said, all the build helpers act the same way. We might think in another mechanism to skip/force something?

@agurtovoy
Copy link
Contributor Author

@agurtovoy agurtovoy commented Jan 2, 2020

@lasote I totally understand the desire to keep the environment setup in the common code, but it can be argued that build helpers already violate that rule/make it moot by applying their own changes to the environment — after the common setup has been run — on a conditional, ad hoc basis. It's not immediately apparent to me why the latter is okay, but making it the helper's responsibility to insure that the user-specified environment survives that fiddling is not.

@memsharded
Copy link
Member

@memsharded memsharded commented Jan 30, 2020

Hi @agurtovoy

I have been playing with this here: #6297
Considering to merge it, but I am still afraid that this could break others users that actually want this behavior and rely on the helper for defining the right environment. I wanted to ask. Would you be ok with an opt-in mechanism, something like:

cmake = CMake(..., apply_self_env=True)
cmake.build()

This way we can introduce this functionality being 100% sure we don't break other users.

@agurtovoy
Copy link
Contributor Author

@agurtovoy agurtovoy commented Jan 31, 2020

Hi @memsharded, we would be okay with it since we're always constructing build helpers manually, and I totally understand the desire to ensure backward compatibility, but it seems to me that the fix has the possibility of breaking something only if the user is using Conan's [env] section to override/add environment variables, and if they are, then, in accordance to the current docs, they expect these overrides to work. Having to explicitly opt-in to what is currently documented as default behavior seems off to me.

Hope this makes sense, and thanks for all your work.

@memsharded memsharded requested a review from jgsogo Jan 31, 2020
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Jan 31, 2020

Hi, although I approved #6297, after thinking about this issue and sharing my POV with some other team members I've changed my opinion and, thinking about the long term, I think that the environment from the profile cannot modify the behavior of our build helpers.

(Sorry if it sounds a little bit opinionated, I'm open to talk about it, of course, but I want to make sure it is clear) 👨‍👧‍👧

IMHO a Build helper is the implementation of a commitment between the Conan settings and the build system, same inputs should provide the same outputs and we should do our best to keep this rule of thumb. Environment variables are a hidden way to modify the build helper behavior, if we let the user change some behavior (choosing a different compiler, for example) we are like hacking the build helper.

I don't like hidden environment variables, but I would support having a way to pass some attributes (even environment variables) like the one that @memsharded is pointing out, nevertheless I would force it to be even more explicit:

cmake = CMake(...)
...
env = {... build your dictionary here... }
cmake.build(env=env)

and this env can be applied just before running the actual command (after applying the env from the dependencies and the env from the build-helper itself).

I think that any move we do from now on should take into account the behavior we want for next Conan major release and without breaking we should solve the bugs and implement the new features having that future release in mind.

@agurtovoy I need to have a look at the docs and see if this is too much breaking according to them. Maybe if we agree on this behavior the bug is on the docs and we can fix them to explain better the expected behavior (and the intention for the next big release)

Copy link
Member

@jgsogo jgsogo left a comment

Yes, I like it as it is explicit in the interface.

My only concern is if we want to modify the interface of the tool.environment_append tool, I'd say no and I would modify the tools.vcvars instead (maybe we need a private _environment_append(...., post=False) to be called from environment_append(env_vars) and from vcvars(...., post=False)). wdyt?

@@ -30,13 +30,14 @@ def run_environment(conanfile):


@contextmanager
def environment_append(env_vars):
def environment_append(env_vars, post=False):
Copy link
Member

@jgsogo jgsogo Feb 3, 2020

Choose a reason for hiding this comment

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

Do we want to add it to the tool and to expose the new behavior to the user? (no one has asked for it so far)

env_build = AutoToolsBuildEnvironment(self._conanfile)
with environment_append(env_build.vars):
self._conanfile.run(command)

if self._vcvars_needed:
vcvars_dict = tools.vcvars_dict(self._settings, output=self._conanfile.output)
with environment_append(vcvars_dict, post=self._append_vcvars):
Copy link
Member

@jgsogo jgsogo Feb 3, 2020

Choose a reason for hiding this comment

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

Maybe the post=self._append_vcvars makes sense as an argument to with tools.vcvars(..., post=self._append_vcvars) and keep environment_append unmodified.

self.generator in ["Ninja", "NMake Makefiles", "NMake Makefiles JOM"]):
vcvars_dict = tools.vcvars_dict(self._settings, force=True, filter_known_paths=False,
output=self._conanfile.output)
with environment_append(vcvars_dict, post=self._append_vcvars):
Copy link
Member

@jgsogo jgsogo Feb 3, 2020

Choose a reason for hiding this comment

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

Maybe the post=self._append_vcvars makes sense as an argument to with tools.vcvars(..., post=self._append_vcvars) and keep environment_append unmodified.

@memsharded memsharded merged commit 048da96 into conan-io:develop Feb 3, 2020
2 checks passed
@memsharded
Copy link
Member

@memsharded memsharded commented Feb 4, 2020

Hi @agurtovoy

Please check the description:

Changelog: Feature: Adds vcvars_append variable (defaulting to False) to CMake and Meson build helpers constructors, so when they need to activate the Visual Studio environment via vcvars (for Ninja and NMake generators), the vcvars environment is appended at the end, giving precedence to the environment previously defined.

As the risk of breaking was still there, and there were discussions if the build helper should actually have precedence, we have considered it an opt-in feature at the moment. Will probably change the default in Conan 2.0, but at the moment, better not risk.

@agurtovoy
Copy link
Contributor Author

@agurtovoy agurtovoy commented Feb 5, 2020

Hi @memsharded, the description looks good to me. Thanks again for yours and your team's work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants