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

Enhance vs-generator #5564

Merged
merged 1 commit into from Sep 9, 2019
Merged

Enhance vs-generator #5564

merged 1 commit into from Sep 9, 2019

Conversation

@kobalicek
Copy link
Contributor

@kobalicek kobalicek commented Jul 31, 2019

Changelog: Feature: Enhanced vs-generator by providing more properties that can be referenced by other projects; added library paths also to so it's possible to compile static libraries that reference other libs
Docs: Omit

Close #5561

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Jul 31, 2019

CLA assistant check
All committers have signed the CLA.

@memsharded
Copy link
Member

@memsharded memsharded commented Aug 1, 2019

Hi @kobalicek

Thanks very much for contributing this. It is looking good, just 3 tests are broken because they check the output of the visual_studio generator. Can you please have a look and fix them? Otherwise just tell and we will help with that, no problem.

@memsharded memsharded added this to the 1.19 milestone Aug 1, 2019
@memsharded
Copy link
Member

@memsharded memsharded commented Aug 1, 2019

Side note, @kobalicek have a look to my edits to the first comment in the PR. We use that format to automatically generate the changelog, and it will also automatically close that issue. Thanks again!

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Aug 1, 2019

I see that this PR is adding some ; at some places to concatenate elements:

  • $(ConanIncludeDirectories);%(AdditionalIncludeDirectories)
  • $(ConanPreprocessorDefinitions);%(PreprocessorDefinitions)
  • $(ConanLibraryDirectories);%(AdditionalLibraryDirectories)
  • $(ConanPreprocessorDefinitions);%(PreprocessorDefinitions)

Conan provided elements already ends with ;:

fields = {
            'condition': condition,
            'bin_dirs': "".join("%s;" % p for p in build_info.bin_paths),
            'res_dirs': "".join("%s;" % p for p in build_info.res_paths),
            'include_dirs': "".join("%s;" % p for p in build_info.include_paths),
            'lib_dirs': "".join("%s;" % p for p in build_info.lib_paths),
            'libs': "".join(['%s.lib;' % lib if not has_valid_ext(lib)
                             else '%s;' % lib for lib in build_info.libs]),
            'definitions': "".join("%s;" % d for d in build_info.defines),
            'compiler_flags': " ".join(build_info.cxxflags + build_info.cflags),
            'linker_flags': " ".join(build_info.sharedlinkflags),
            'exe_flags': " ".join(build_info.exelinkflags)
        }

I suppose there is no problem having ;; (an empty element or two ; at the end), but I would suggest to fix this too or keep previous behavior. The fix would be to simplify the join statements above to something like: 'bin_dirs': ";".join(build_info.bin_paths),.

@memsharded , shall we change this too or just keep previous behavior?

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 1, 2019

@memsharded Unfortunately I noticed the tests were failing after the PR. Initially, I didn't want to touch anything else to not break some other part of the generator. I have looked at the tests and I would have to spend some time figuring out how to fix them (I'm not really a python guy). We initially only tested with our project.

I now see the problem with semicolons and I think they should:

  • either be removed from template vars and then just used as $(ConanIncludeDirectories);%(AdditionalIncludeDirectories) (which is probably breaking behavior)
  • kept and then used as $(ConanIncludeDirectories)%(AdditionalIncludeDirectories)

Both have pros/cons so let me know your preference.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Aug 1, 2019

@kobalicek, let's decide what to do with the ; and then I will help you with the Python tests ;D

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 1, 2019

I would prefer the second option as it's backwards compatible, but in the end it doesn't matter for us :)

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 2, 2019

Should I update the PR to use the second option and a proper commit message?

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Aug 2, 2019

Yes, please, let's go for the more conservative option, but add a comment in the code like FIXME Conan 2.0, use proper ';' (maybe link to these last comments) so we realize to improve it in the future.

Do not worry about the commit message, we stash changes before merging to develop.

Thanks

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 2, 2019

Not sure why the tests are not running for the latest push, did I reach some limit?

I cannot run the tests from here so I rely on CI to finish this.

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Aug 2, 2019

I can see it running in the CI right now, maybe it was due to the force push that Jenkins was not able to ping back Github, but now it is working. Thanks!

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 2, 2019

OK it's finally passing, let me know about possible further improvements.

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 6, 2019

Ping :) Any objections / suggestions?

Copy link
Member

@jgsogo jgsogo left a comment

I really like this new approach, it is more flexible and it is also solving some issues:

  • We had <ConanResourceDirectories>{res_dirs};%(ConanResourceDirectories)$(</ConanResourceDirectories> with a very wrong $( there 👍
  • We are removing %(ConanBinaryDirectories) and %(ConanResourceDirectories) as evaluated values in the first PropertyGroup, I'm not sure if they could be taking any value from an inherited project and then we are breaking (or not allowing) some behavior.

I think that it is probably worth an actual test with VS to check that the values are evaluated and taken into account to compile the project.

We have still a couple of weeks before closing the release, time enough to check all of this.

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 13, 2019

We have already tested this change locally, but I would welcome somebody else to test this independently of course.

@toughengineer
Copy link

@toughengineer toughengineer commented Aug 13, 2019

Hello all.
I also need this bug to be fixed. 🙂
In my case I have to manually specify Additional Dependencies as well as Additional Dependency Directories in Librarian properties, so vcxproj section looks like:

<Lib>
  <AdditionalDependencies>xsec_2_0.lib</AdditionalDependencies>
  <AdditionalLibraryDirectories>$(Conan-xmlsecurity-Root)/lib;%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
</Lib>

@kobalicek, in your changes I see that you only added AdditionalLibraryDirectories in the Lib section unless I'm missing something, so I suppose you need to add something like

<AdditionalDependencies>{libs}%(AdditionalDependencies)</AdditionalDependencies>

to the Lib section as well.

@kobalicek
Copy link
Contributor Author

@kobalicek kobalicek commented Aug 15, 2019

@toughengineer I didn't want to change so much so I did only change that would be backward compatible. It's true that maybe <AdditionalDependencies> should be populated as well and I thought about it, but then reconsidered to make this more likely of being merged.

@jgsogo Should I change the PR and add <AdditionalDependencies> also to <Lib>?

@toughengineer
Copy link

@toughengineer toughengineer commented Aug 15, 2019

@kobalicek, the Lib section was missing altogether, hence the addition of AdditionalDependencies should not affect compatibility in any way. So, to be precise, it should look something like this:

<Lib>
  <AdditionalLibraryDirectories>$(ConanLibraryDirectories)%(AdditionalLibraryDirectories)</AdditionalLibraryDirectories>
  <AdditionalDependencies>{libs}%(AdditionalDependencies)</AdditionalDependencies>
</Lib>

If the generated props file does not specify it, I believe you would have to manually specify the libs to link against somewhere anyway, which is not what we want.

Enhanced vs-generator by providing more properties that can be referenced by other projects; added library paths also to <Lib> so it's possible to compile static libraries that reference other libs
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Aug 22, 2019

This is going to be merged to 1.19 which will be released by the end of this month... so just a little bit more patience. 👍

@jgsogo jgsogo requested a review from danimtb Aug 22, 2019
Copy link
Member

@danimtb danimtb left a comment

Everything looks fine from my side! Thanks for the PR 😄

@memsharded memsharded merged commit 6d1ff50 into conan-io:develop Sep 9, 2019
2 checks passed
@memsharded
Copy link
Member

@memsharded memsharded commented Sep 9, 2019

Merged, will be in next 1.19 release.

Thanks @kobalicek for the contribution and @toughengineer for the feedback!

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

Successfully merging this pull request may close these issues.

6 participants