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

fixing virtualbuildenv #4583

Merged
merged 3 commits into from Feb 26, 2019

Conversation

Projects
None yet
4 participants
@memsharded
Copy link
Contributor

commented Feb 20, 2019

Changelog: Bugfix: Fix LIB overwrite in virtualbuildenv generator
Docs: omit

Close #4582

This PR removes 2 files that were not named "_test" in the test scope, and then not being executed. Please @danimtb check.

@lasote lasote added this to the 1.13 milestone Feb 21, 2019

@lasote lasote assigned lasote and unassigned memsharded Feb 21, 2019

@lasote lasote requested a review from danimtb Feb 21, 2019

@danimtb
Copy link
Member

left a comment

Removing those files
Don't you have a test to prove the fix? Could be a unit test

@ghost ghost assigned memsharded Feb 21, 2019

@memsharded memsharded removed their assignment Feb 21, 2019

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Yes, I already added a test, not a unittest but a integration one, because it happened to be quite broken (and unittest doesn't catch those things), 2 things have to be fixed:

  • Not overwriting the lib paths
  • The _LINK_ env var is path separated

Please @lasote review.

@lasote lasote merged commit 2da6846 into conan-io:develop Feb 26, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Feb 26, 2019

@memsharded memsharded deleted the memsharded:feature/fix_virtualbuildenv branch Feb 26, 2019

@jmoutray

This comment has been minimized.

Copy link

commented Mar 4, 2019

(Windows) In trying to work around the issue until 1.13 is released, I discovered that the LIB environment variable "does not apply" unless UseEnv=true: You must do "SET UseEnv=True" (if launching Visual Studio from conanbuildenv prompt) or pass argument "/p:UseEnv=true" (if executing msbuild). See Remarks at https://docs.microsoft.com/en-us/visualstudio/ide/reference/useenv-devenv-exe?view=vs-2017.

I'm not sure what this means for conan. Maybe activate_build.bat should set this env var.

I am trying to use virtualbuildenv in lieu of updating dependent Visual Studio projects to reference the "conanbuildinfo_multi.props" generated property file.

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Thanks for the info @jmoutray
The truth is that the test is done with:

self.run('activate_build.bat && cl /c /EHsc hello.cpp')
self.run('activate_build.bat && lib hello.obj -OUT:helloapp.lib')
self.run('activate_build.bat && cl /EHsc main.cpp helloapp.lib')

and it works fine, but yes, it is not the VS MSbuild system

The MSBuild conan helper is already setting '/p:UseEnv=true', so I guess it should be only the env-var.

We might try to have a look, but I don't think we will make it for Conan 1.13, because it is going to be released very soon, so we might need to wait.

I am trying to use virtualbuildenv in lieu of updating dependent Visual Studio projects to reference the "conanbuildinfo_multi.props" generated property file.

I am not sure how far can you get doing this, because the conanbuildinfo_multi.props works nicely for switching Debug/Release, so if developers are going to use it, it might be more ergonomic than playing with the environment

@memsharded memsharded referenced this pull request Mar 4, 2019

Merged

adding the env-var #4655

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Mar 4, 2019

Trying to test in #4655, to see what happens with CI.

@jmoutray

This comment has been minimized.

Copy link

commented Mar 5, 2019

One more... There doesn't appear to be any way to pass the winsdk_version to use for the vcvars command for the virtualbuildenv generator. Using MSBuild, I can do this: msbuild.build("filename.sln", winsdk_version="8.1")

Without the "8.1" argument, I get a linker error:
LINK : fatal error LNK1158: cannot run 'rc.exe'

More info at https://stackoverflow.com/questions/14372706/visual-studio-cant-build-due-to-rc-exe#14373113.

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

Yes, that might be an issue. As commented above, the virtualbuildenv, in case of Visual Studio/MSBuild might be a bit limited, and because of those limitations are inherent to VS/MSbuild, there is nothing that Conan could do.

In any case, the files created by generators are not to configure the build, they convey information strictly from dependencies. The build helpers like MSBuild() can try to inject some things in the command line, but the generated files like activate_build.bat are not intended to map build configuration, but mostly information from dependencies like include paths, library paths, library names, etc.

@memsharded

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@jmoutray

The UseEnv=True has been added in #4655. It will be released next Conan 1.16, please update when it is released and give feedback if it doesn't work.

If the issue about winsdk is still not solved, I suggest to open a new issue for it, it is difficult to track comments in closed PRs. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.