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

Regression of other extensions for Visual Studio (#5316) #5319

Merged

Conversation

Projects
None yet
4 participants
@uilianries
Copy link
Member

commented Jun 7, 2019

  • Regrets #5254 to fix #5316
  • Add new tests to validate any possible regression

Changelog: Fix: Accept only .lib and .dll as Visual extensions (#5316)
Docs: Omit

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

@tags: slow

@jgsogo jgsogo self-requested a review Jun 7, 2019

Show resolved Hide resolved conans/client/generators/visualstudio.py Outdated
Show resolved Hide resolved conans/client/build/visual_environment.py Outdated
Show resolved Hide resolved conans/test/unittests/client/generators/visual_studio_multi_test.py Outdated
@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jun 8, 2019

@memsharded could you confirm if we should accept .dll as extension or just .lib is fine?

@memsharded memsharded changed the base branch from develop to release/1.16.1 Jun 9, 2019

@memsharded memsharded changed the base branch from release/1.16.1 to develop Jun 9, 2019

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

Hi @uilianries

Please do this PR to release/1.16.1 branch, it is a regression, should be fixed asap. Many thanks!

@memsharded
Copy link
Contributor

left a comment

Target 1.16.1

@memsharded memsharded added this to the 1.16.1 milestone Jun 9, 2019

@uilianries uilianries changed the base branch from develop to release/1.16.1 Jun 9, 2019

@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

I think is better recreating a new PR, because I started this one from develop branch.

uilianries added some commits Jun 7, 2019

#5316 Regression for other Visual extensions
- Regrets #5254 to fix #5316

Signed-off-by: Uilian Ries <uilianries@gmail.com>
#5316 Rename extension method
Signed-off-by: Uilian Ries <uilianries@gmail.com>
#5316 Update MSVC test
- Remove duplicated code

Signed-off-by: Uilian Ries <uilianries@gmail.com>
#5316 Fix Visual Studio multi test
Signed-off-by: Uilian Ries <uilianries@gmail.com>

@uilianries uilianries force-pushed the uilianries:hotfix/visual-lib-extension branch from 36c8a12 to e5871cd Jun 9, 2019

@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

forced push because I moved the base branch to release/1.16.1 and cherry-pick all 4 commits

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

Added tags=slow, and relaunched, as this will break the new test introduced in #5254. Unfortunately, the mylib.a is also a valid extension, used by some users, and it totally work, that test proves it.

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 9, 2019

My bad, it is not broken, but the test is not running, because the filename is not adding _test. I will fix it asap.

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

The CI is broken, this PR needs to be relaunched after fixing it

@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

I have pushed changes, adding ".a" to the set of valid extensions for the generator.

@jgsogo

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

I think that if we are going to change the extension list (previously it was only .lib) it should be the same we are using in collect_libs, somehow this two methods (collect and generate) are related:

if ext in (".so", ".lib", ".a", ".dylib", ".bc"):

collect_libs is removing some extensions, and generators are adding them again. Given this, I will limit the list to .lib and .a.

With this change, we are breaking users with library names like mylib.a.lib (using cpp_info.libs = ["mylib.a"]), although we are helping some users with custom library extensions:

  • .a: collect_libs is working for them, now they won't get .a.lib in the generator. Was that change (#5254) motivated by any issue? Nevertheless, I agree with it even though we may break someone else (no one has complained yet).
  • .dll: collect_libs is not working for them, still not working, but they can start to use the generator. <-- who uses .dll as a custom suffix for import libraries? 🤷‍♂ Please, reconsider.

Any other custom exception will still be broken because the generator will add .lib to it although collect_libs won't trim it (comment here) .

@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

I think that if we are going to change the extension list (previously it was only .lib) it should be the same we are using in collect_libs

I think it's a good idea. We could re-use the same code from there.

#5316 Using collect libs extensions
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@memsharded

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

collect_libs is removing some extensions, and generators are adding them again. Given this, I will limit the list to .lib and .a.

Not true, collect_libs is removing all extensions (the last one only in case of several extensions). For those extensions, it is also removing the lib prefix.

@jgsogo

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

collect_libs is removing some extensions, and generators are adding them again. Given this, I will limit the list to .lib and .a.

Not true, collect_libs is removing all extensions (the last one only in case of several extensions). For those extensions, it is also removing the lib prefix.

Not exactly, it adds the file only if it has one of those extensions, and removes the lib prefix for all but those with suffix .lib. This is the current implementation:

if ext in (".so", ".lib", ".a", ".dylib", ".bc"): 
    if ext != ".lib" and name.startswith("lib"):
        name = name[3:]
    # ... do things
    result.append(name)
Show resolved Hide resolved conans/client/generators/visualstudio.py Outdated
#5316 Export valid library extensions
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

@jgsogo

jgsogo approved these changes Jun 10, 2019

Copy link
Member

left a comment

I would suggest some changes, but I'm ok with this, I don't want to delay the minor.


These changes can be done once this PR arrive to develop:

  • No need for a function, valid_lib_extensions can be a static list/tuple
  • About exposing it to conans.tools.valid_lib_extensions, right now I don't find it useful for the user (and it is not documented).
@uilianries

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

About exposing it to conans.tools.valid_lib_extensions, right now I don't find it useful for the user (and it is not documented).

What about using as protected?

Show resolved Hide resolved conans/tools.py Outdated
Show resolved Hide resolved conans/client/tools/files.py Outdated
#5316 Export lib ext as tuple
Signed-off-by: Uilian Ries <uilianries@gmail.com>
#5316 Add extra test case
Signed-off-by: Uilian Ries <uilianries@gmail.com>

@memsharded memsharded merged commit e45d68c into conan-io:release/1.16.1 Jun 10, 2019

2 checks passed

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

@memsharded memsharded referenced this pull request Jun 10, 2019

Closed

[Regression] Two dots in lib filename #5316

3 of 3 tasks complete
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.