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

[bug] VirtualBuildEnv + VirtualRunEnv with scope=build, deactivate_conanbuild.bat does not properly restore the PATH #13693

Closed
benjaminc-mirageoscience opened this issue Apr 14, 2023 · 8 comments · Fixed by #13707
Assignees
Milestone

Comments

@benjaminc-mirageoscience

Environment details

  • Operating System+version: Windows 10
  • Compiler+version: Visual Studio 16 2019
  • Conan version: 1.59
  • Python version: 3.9.4

conanfile.py:

from conan import ConanFile
from conan.tools.microsoft import vs_layout, MSBuildDeps
from conan.tools.env import VirtualBuildEnv, VirtualRunEnv


class MyConan(ConanFile):
    settings = "os", "compiler", "build_type", "arch"
    package_type = "application"

    default_options = {
        "qt/*:shared": True,
        "qt/*:commercial": False,
        "qt/*:qttools": True,
        "doxygen/*:enable_search": False
    }
    
    def requirements(self):
        self.requires("qt/5.15.8")

    def build_requirements(self):
        self.tool_requires("doxygen/1.9.4")

    def layout(self):
        vs_layout(self)

    def generate(self):

        ms = MSBuildDeps(self)
        ms.generate()

        ms = VirtualBuildEnv(self)
        ms.generate()

        ms = VirtualRunEnv(self)
        ms.generate(scope="build")

Host and build profile (the same), conanprofile.txt:

[settings]
os=Windows
os_build=Windows
arch=x86_64
arch_build=x86_64
compiler=Visual Studio
compiler.version=16
compiler.toolset=v141
cppstd=17
build_type=Release

[conf]
tools.env.virtualenv:powershell=False

Steps to reproduce

  1. Be in the folder containing conanfile.py and conanprofile.txt.
  2. Run conan install . --lockfile="" --profile:host=conanprofile.txt --profile:build=conanprofile.txt --install-folder=build --output-folder=build --build=missing
  3. At this stage, doxygen.exe and moc.exe are not available (not in the PATH)
  4. Run build\conan\conanbuild.bat
  5. At this stage, doxygen.exe and moc.exe are available (in the PATH)
  6. Run build\conan\deactivate_conanbuild.bat
  7. At this stage, doxygen.exe is still available in the PATH (moc.exe is not as expected). doxygen.exe should not be available.

Logs

image

Content of conanbuild.bat:

@echo off
call "%~dp0/conanbuildenv-release-x86_64.bat"
call "%~dp0/conanrunenv-release-x86_64.bat"

Content of deactivate_conanbuild.bat:

@echo off
call "%~dp0\deactivate_conanbuildenv-release-x86_64.bat"
call "%~dp0\deactivate_conanrunenv-release-x86_64.bat"

In order to restore the PATH properly, the call order in deactivate_conanbuild.bat should be the reverse of the call order in conanbuild.bat. The content of conanbuild.bat should not change, but the content of deactivate_conanbuild.bat should be:

@echo off
call "%~dp0\deactivate_conanrunenv-release-x86_64.bat"
call "%~dp0\deactivate_conanbuildenv-release-x86_64.bat"

I did not try but maybe there is a similar issue when using VirtualBuildEnv with scope=run + VirtualRunEnv.

This is some context about why I needed this. My application depends on Qt, so I needed it as a requirement. In addition, at build time, some Qt tools are needed: moc.exe, rcc.exe, lupdate.exe, and lrelease.exe. Unfortunately, the Qt binary path is not added to the PATH when I use the VirtualBuildEnv generator (it does with the VirtualRunEnv generator). I could just use the VirtualRunEnv generator, but I also use doxygen to generate my code documentation. I set it as a tool_requires. Doxygen binary path is put in the PATH with the VirtualBuildEnv generator but not with the VirtualRunEnv generator. To generate a .qch documentation, a Qt binary is necessary: qhelpgenerator.exe. My problem is that the doxygen binary directory was only set in the PATH with the VirtualBuildEnv generator, and the Qt binary path only with the VirtualRunEnv generator. I need both in the PATH.

An option I tried was to define Qt as a tool requirement (tool_requires) as the same time as a regular requirement. It worked but that installed 2 different Qt packages. My understanding is that one is for the build context and the other one for the host context. In my case, the build context and the host context are the same (same conan profile) so I would like to avoid the use of 2 different packages. That is why I set the scope to build when I used the VirtualRunEnv generator. Let me know if there is a better approach.

Thank you for your help.

@memsharded memsharded self-assigned this Apr 17, 2023
@memsharded memsharded added this to the 2.0.5 milestone Apr 17, 2023
@memsharded
Copy link
Member

Hi @benjaminc-mirageoscience

Thanks for your detailed report.
I think this might be somewhat related: #10979. Not exactly the same, but there is some intention in #12914 by @RubenRBS to replace the de-activation scripts by other approaches.

In any case, this is what we have in the code:

    def deactivates(filenames):
        # FIXME: Probably the order needs to be reversed
        result = []
        for s in filenames:

This should be relatively straightforward to fix, lets try for next 2.0.5.

@memsharded
Copy link
Member

Did PR #13707 to fix it.

Regarding your other comment:

An option I tried was to define Qt as a tool requirement (tool_requires) as the same time as a regular requirement. It worked but that installed 2 different Qt packages. My understanding is that one is for the build context and the other one for the host context. In my case, the build context and the host context are the same (same conan profile) so I would like to avoid the use of 2 different packages. That is why I set the scope to build when I used the VirtualRunEnv generator. Let me know if there is a better approach.

This is not the best approach. It will fail in any cross-build scenario. If you are doing native build with same host and build profile, they will use the same package_id, so the package binary will be the same, so this shouldn't be an issue.
If you want to further discuss it, it would be better to create a dedicated issue for this, as #13707 will close this issue. Thanks!

@memsharded
Copy link
Member

Closed by #13707, will be released next 2.0.5.

Please see my other comment above to discuss about the tool_requires.

@benjaminc-mirageoscience
Copy link
Author

Closed by #13707, will be released next 2.0.5.

Please see my other comment above to discuss about the tool_requires.

Thank you @memsharded for your reply and fix! Nice that it will be fixed in 2.0.5. I guess that it will be not fixed in 1.60? I am asking as I am still using Conan 1 (with the back-ported Conan 2 features) as many 3rd parties I need do not have their recipes ported to Conan 2 yet.

@memsharded
Copy link
Member

At the moment, this kind of fix that is not severe (there are workarounds, you can keep using 1.X for a while, this bug could be inconvenient but not a big blocker) and are not regressions are difficult to prioritize for 1.X releases, too many things to do (for example, a large part of the team is working hard to keep porting ConanCenter recipes to be 2.0 ready). If it is a big blocker, please let us know. Thanks!

@benjaminc-mirageoscience
Copy link
Author

At the moment, this kind of fix that is not severe (there are workarounds, you can keep using 1.X for a while, this bug could be inconvenient but not a big blocker) and are not regressions are difficult to prioritize for 1.X releases, too many things to do (for example, a large part of the team is working hard to keep porting ConanCenter recipes to be 2.0 ready). If it is a big blocker, please let us know. Thanks!

I understand. No, this is not blocking at all. Thank you for all your work in the Conan project.

@memsharded
Copy link
Member

Thanks to you for the report and the feedback! 😃

@benjaminc-mirageoscience
Copy link
Author

Did PR #13707 to fix it.

Regarding your other comment:

An option I tried was to define Qt as a tool requirement (tool_requires) as the same time as a regular requirement. It worked but that installed 2 different Qt packages. My understanding is that one is for the build context and the other one for the host context. In my case, the build context and the host context are the same (same conan profile) so I would like to avoid the use of 2 different packages. That is why I set the scope to build when I used the VirtualRunEnv generator. Let me know if there is a better approach.

This is not the best approach. It will fail in any cross-build scenario. If you are doing native build with same host and build profile, they will use the same package_id, so the package binary will be the same, so this shouldn't be an issue. If you want to further discuss it, it would be better to create a dedicated issue for this, as #13707 will close this issue. Thanks!

Hi @memsharded . Sorry about the long delay to reply to you about this. So I tried to set Qt as a regular requirement and also as a tool requirement. As I used the same profile for the build and host contexts, I expected Conan to use the same package of Qt. But I had two (in the VS .props files generated by Conan). I tried to understand what happened. Not sure I fully understood but it seems that the Qt package used for the host context was the one from a custom remote I used, and the Qt package used for the build context was one that I had to rebuild (conan install asked to rebuild).

I disabled the custom remote I used, removed all the Qt packages I had on my machine and retried. Eventually, Conan used the same Qt package for both contexts. Not sure what was wrong with the Qt package from the remote I used. Anyway, now it works fine. Thanks for your help.

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

Successfully merging a pull request may close this issue.

2 participants