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

PoC of simplified environment management #8426

Closed
wants to merge 13 commits into from

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Feb 1, 2021

Changelog: Feature: New environment management, with correct value capturing and restoring on deactivate
Docs: Omit

New approach to environment (env-vars) management:

  • All operations are explicit: define, append, prepend, clean. No implicit if it is a list, then...
  • It can do any accumulation pattern over the existing env-var, like both prepend and append values
  • Composition is also very explicit, concise and well defined.
  • Lets make value separator explicit (TODO: Implement a "pathsep" placeholder.)
  • Less magic of hardcoded variables that should be appended with spaces or with path separator
  • Can also handle case-insensitivity in Windows, doing exactly the same operation that user will do in cli.
  • A new approach to generate "deactivate" files, that do not depend on the moment of creating the file, but on the moment that the "activate" script is launch
  • Plan to use this extensively in Conan toolchains. Also in profile and requires env_info, superseding the previous one (might be a parallel syntax).
  • Environment management in Conan recipes must be always explicit. By default Conan methods do not activate anything in the environment, and either generate() should create files, or recipes activate what they want more explicitly.

TODO:

  • Powershell support
  • Pathsep placeholder

@memsharded
Copy link
Member Author

@solvingj I'd like to know your feedback about this, and also if you know powershell, would you mind contributing it too?

@solvingj
Copy link
Contributor

solvingj commented Feb 9, 2021

I'd love to participate. I know powershell pretty well. Can we write up a description of "the big idea" here? What are the goals, how is it different from previous environment management, what are biggest challenges, what do we still need to do?

@solvingj
Copy link
Contributor

solvingj commented Feb 9, 2021

Ok, based on updated description, Ihere's what I would imagine some future generate() method looking like... is it close to what you imagined?

replace this:

    with tools.environment_append(dict):
            mt = MakeToolchain()
            mt.generate()

With something like this (I could imagine multiple different syntax choices here):

    et = EnvToolchain(self, name="buildvars")
    et.variables["somevar"] = "someval"
    et.generate() # -> buildvars.env, activate_buildvars/deactivate_buildvars (.bat, sh, ps1, py)

    mt = MakeToolchain()
    mt.with_env_file("buildvars")
    mt.generate()

@solvingj
Copy link
Contributor

Thought about this more. From the perspective of a naïve recipe author who wants/needs full control of "the build of the current recipe" because they need to do non-standard things with the environment variables to make some non-standard recipe work, the a completely explicit approach seems like a nice and safe and powerful strategy. From the perspective of a caller who is building many packages, and wants to set the CC/CXX environment variables for all possible build tools in the dependency tree which support it, it seems completely unreasonable that recipes are highly likely to fail to pass Conan's environment variables to toolchains.

Indeed, it seems that there must still be some implicit generation of an environment file with a default name, and the other toolchains should implicitly load it. The [conf] mechanism might be a suitable mechanism for callers to "opt-out" of the implicit generation/load on a per-package basis (when they discover recipes in their dependency like the one described above, where the default/implicit environment won't work, and the recipe defines some custom mutation/modification/filter of the environment).

@memsharded
Copy link
Member Author

@klimkin, it seems you know very well about bash and environment, if you could give quick feedback here, would be great :).

What is the effect of not providing such ${var:+...] in shell scripts? From what I see in the tests, it seems the worst an extra blank or path separator would be appended, but in practical terms I don't know a real case that this would produce any bad behavior. I'd like to keep the code as simple and explicit as possible, and considering not adding such expansion.

@klimkin
Copy link
Contributor

klimkin commented Feb 13, 2021

Extra spaces in command lines or extra ":" in path variables are harmless for shell, but can have some undesired side effects.

  • For example, user script might not necessary be able to parse ":path:path:" correctly if using a primitive split on ":".
  • Another example is having a space at the end of output line. The text just looks the same, but diff would fail.

For fundamental tools like Conan, I think it would be nice to keep this clean. By clean I mean to not have extra space/colon where it's not required.

@memsharded
Copy link
Member Author

For fundamental tools like Conan, I think it would be nice to keep this clean. By clean I mean to not have extra space/colon where it's not required.

Yes, I agree it is a nice goal, I am trying to estimate the value vs complexity ratio. Specially because in other platforms, like .bat batch scripts in Windows this is not possible in this way (such expansion doesn't exist), and it would be required to have very complicated code just to avoid this extra space or path separator.

@klimkin
Copy link
Contributor

klimkin commented Feb 13, 2021

Yes, I agree. And there also shell flavors like csh still around that don't understand bash notation. I agree, if simple form works (and it does), that would be ok.

@memsharded
Copy link
Member Author

Superseded by #8534, please track progress in that other PR.

@memsharded memsharded closed this Feb 24, 2021
@memsharded memsharded deleted the feature/environment branch March 11, 2021 23:06
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.

3 participants