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

#4696: Build-Mode pattern now covers the whole reference #4787

Merged
merged 8 commits into from Mar 25, 2019
Merged

#4696: Build-Mode pattern now covers the whole reference #4787

merged 8 commits into from Mar 25, 2019

Conversation

@monsdar
Copy link
Contributor

@monsdar monsdar commented Mar 20, 2019

Fixes #4696

Changelog: Feature: --build parameter now applies fnmatching onto the whole reference, allowing to control rebuilding in a much broader way.
Changelog: Bugfix: --build parameter now is always case-sensitive, previously it depended to the file system type.
Docs: conan-io/docs#1141

@CLAassistant
Copy link

@CLAassistant CLAassistant commented Mar 20, 2019

CLA assistant check
All committers have signed the CLA.

@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 20, 2019

Not sure if this is considered breaking:
When someone uses conan install foo/1.0@user/stable --build B*ost

  • It was matching the dependency Boost/1.69.0@user/host
  • It wasn't matching the dependency Bar/1.0.0@user/host

With the new implementation it will take the whole package reference an fnmatches it. Therefore the new implementation will match both references. This is a different behaviour like before.

The implementation works like expected with things like Boost* or similar though. It's just different when * is placed within the middle of the build_mode pattern.

@lasote lasote added this to the 1.14 milestone Mar 20, 2019
@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 20, 2019

Not sure where the docs need to be expanded tbh... This feature is kind of cluttered throughout the documentation.

if fnmatch.fnmatch(ref.name, pattern) or repr(ref) == pattern:
is_matching_pattern = fnmatch.fnmatch(repr(ref), pattern)
is_exact_match = (repr(ref) == pattern)
is_same_component = repr(ref).startswith(pattern)
Copy link
Contributor

@lasote lasote Mar 20, 2019

Shouldn't this be also fnmatch (the old one actually)? That it means that if I use --build=p will work with any ref starting with p? Or am I missing something?

Copy link
Contributor

@lasote lasote Mar 20, 2019

Add an assert of a parameter without 🌟

Copy link
Contributor Author

@monsdar monsdar Mar 20, 2019

Line 58 actually checks if the given reference starts with the pattern. So the check will return true in your case with p.

Copy link
Contributor Author

@monsdar monsdar Mar 20, 2019

Added a test that shows how this is still working. See BuildModeTest::test_partly_build_force

Copy link
Contributor Author

@monsdar monsdar Mar 20, 2019

Added additional checks for parts of the name

Copy link
Contributor Author

@monsdar monsdar Mar 21, 2019

Removed the feature where p will match pattern or post as discussed below. Not sure if I understood wrong from your message or what your intention was. Let me know when there's something still missing... So far it looks like all tests pass (added some more tests as well).

@lasote
Copy link
Contributor

@lasote lasote commented Mar 20, 2019

We are going to need a PR for the docs also. Thanks!

@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 20, 2019

PR for the docs: conan-io/docs#1130

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 20, 2019

I'm not sure about (examples taken from the tests) "H" matching "Hey/0.1@user/testing" or "*Tool" matching "PythonTool/0.1@lasote/stable".... I would expect those patterns to match only for the exact ref.name (like it was before, I think), so:

  • "Hey" matches "Hey/version@user/channel" (no need for wildcards if it is equal to the ref.name)
  • "*Tool" matches "name/version@user/whateverTool"

But It is just an opinion, I haven't checked previous behavior...

@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 21, 2019

Supporting *Tools and H is the current behaviour. The unittest I added for *Tools is already part of the Jenkins tests.

@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 21, 2019

Jenkins still not passing all tests. Tbh I'm out of ideas what the test-output should tell me and how to make sure the tests pass. Could someone from your side help me out what the issue is and what needs to be done in order to have that solved?

@jgsogo jgsogo self-assigned this Mar 21, 2019
@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 21, 2019

On my way! 👍

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 21, 2019

The test that is failing is associated with one our comments above: "H" should not match "Hey/0.1@user/testing". I think that we only need two lines for this functionality to work:

        for pattern in self.patterns:
            is_matching_name = fnmatch.fnmatch(ref.name, pattern)
            is_matching_ref = fnmatch.fnmatch(repr(ref), pattern)

            if is_matching_name or is_matching_ref:
                 ...

@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 21, 2019

Got it, that's easy to fix then

@jgsogo
Copy link
Member

@jgsogo jgsogo commented Mar 22, 2019

I think now it is ready, I would only add a couple of tests trying to match with different casing... although I'm not sure if casing should be taken into account or not (ping @lasote)

@lasote
Copy link
Contributor

@lasote lasote commented Mar 25, 2019

I would keep the casing as it was (sensitive, right?)

@lasote
Copy link
Contributor

@lasote lasote commented Mar 25, 2019

@monsdar could you add a tiny test to verify that the case is important?

lasote
lasote approved these changes Mar 25, 2019
@lasote lasote merged commit 3600fa1 into conan-io:develop Mar 25, 2019
2 checks passed
@lasote
Copy link
Contributor

@lasote lasote commented Mar 25, 2019

Np. we will add it. THANKS!

@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 25, 2019

Sorry, I haven't had time yet. With the implementation you merged it will check case-insensitive, so this is not working like it should. I changed the implementation to use fnmatchcase instead of fnmatch and added tests for that.

@monsdar
Copy link
Contributor Author

@monsdar monsdar commented Mar 25, 2019

Changes haven't been added, as the PR is already closed. I'll add a new one for the last changes. Feel free to merge that one as well if necessary.

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.

4 participants