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

creating conanvcvars file for toolchains #8719

Merged
merged 3 commits into from Mar 29, 2021

Conversation

memsharded
Copy link
Member

Changelog: Feature: Use conancvvars.bat file for Meson toolchain
Docs: Omit

@memsharded memsharded marked this pull request as ready for review March 29, 2021 14:09
@memsharded memsharded requested a review from SSE4 March 29, 2021 14:11
@memsharded memsharded added this to the 1.35 milestone Mar 29, 2021
@@ -44,7 +36,8 @@ def configure(self, source_folder=None):
cmd += ' "{}" "{}"'.format(self._build_dir, source)
if self._conanfile.package_folder:
cmd += ' -Dprefix="{}"'.format(self._conanfile.package_folder)
self._run(cmd)
vcvars = os.path.join(self._conanfile.install_folder, "conanvcvars")
Copy link
Contributor

Choose a reason for hiding this comment

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

code is repeating 3 times, maybe move it inside self._run instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

self._run might use different locations of environment scripts, depending if they are from Conan or from user ones.

Copy link
Contributor

Choose a reason for hiding this comment

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

and how do we know this locations?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is a generated file, it will be by definition in the install_folder.
If it is a user file, the user will specify its location.

Copy link
Contributor

Choose a reason for hiding this comment

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

but right now it uses self._conanfile.install_folder always, so these 2 lines are repeated.

Copy link
Member Author

Choose a reason for hiding this comment

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

but if we put it inside self._run, it will try to apply it to all the environment scripts, which is incorrect

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean doing:

    def _run(self, cmd):
       vcvars = os.path.join(self._conanfile.install_folder, "conanvcvars")
        self._conanfile.run(cmd, env=["conanbuildenv", vcvars])

are there other environment scripts in meson toolchain? I can't spot any right now

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see what you mean, I was confusing it with self._conanfile.run(). I put some TODOs and some comments there regarding the install or test that I didn't know if they really need the vcvars environment at all. If they need it, then I guess it is ok, if not, each method should define its own environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

not sure, probably best add vcvars everywhere, just in case install needs tools from vcvars (e.g. dumpbin or whatever)

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, it is a small detail, can be refactored later, I am merging this, I prefer to have this in the next release.

@SSE4
Copy link
Contributor

SSE4 commented Mar 29, 2021

are we sure install_folder is always available? I remember it was missing for some cases, e.g. test_package and magic root package

@memsharded memsharded merged commit ba02860 into conan-io:develop Mar 29, 2021
@memsharded memsharded deleted the feature/cvvars_meson branch March 29, 2021 23:14
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.

None yet

2 participants