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

Exclude a package when building all from sources #8483

Merged
merged 19 commits into from Apr 20, 2021

Conversation

uilianries
Copy link
Member

@uilianries uilianries commented Feb 10, 2021

We have 3 different ways to execute it, to illustrate:

Build all from sources, except zlib:

conan install boost/1.75.0@ --build=!zlib --build=*
conan install boost/1.75.0@ --build=!zlib --build

Do not build anything from sources. --build= means '' package, which is invalid.

conan install boost/1.75.0@ --build=!zlib --build=

Changelog: Feature: Skip package when building all package from sources at once using --build=!<package> syntax.
Docs: conan-io/docs#2023
closes #8442

  • 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.

Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.

If Conan install uses --build= it means all packages
must be built. If another package is listed too, it doesn't matter.
e.g conan install foo/0.1@ --build= --build=bar, where bar is
dependency. As --build= listed, all packages will be built from sources

Signed-off-by: Uilian Ries <uilianries@gmail.com>
When passing --build= or --build they should be
the same, but actually they are considered different.
--build= means "", --build means None

However it's not intuitive if you are building a package:
conan install <ref> --build OR
conan install <ref> --build=

Signed-off-by: Uilian Ries <uilianries@gmail.com>
@@ -24,7 +24,7 @@ def __init__(self, params, output):
return

assert isinstance(params, list)
if len(params) == 0:
if len(params) == 0 or "*" in params or "" in params:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If --build, --build= or --build=* is passed, build ALL from sources.

@@ -58,6 +58,8 @@ def __call__(self, parser, namespace, values, option_strings=None): # @UnusedVa
dest.extend(values)
except ValueError:
dest.append(values)
elif namespace == argparse.Namespace(build=[]):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--build= should be consider equal to --build. Otherwise, --build --build=foo will be parsed as --build=foo only.

@uilianries uilianries changed the title [WIP] Exclude a package when building all from sources Exclude a package when building all from sources Feb 10, 2021
@uilianries uilianries marked this pull request as draft February 10, 2021 20:20
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the current behavior is not consistent, and it would be better to fix it, I think that this will be an issue, breaking users that for some reason rely on the current behavior, and unveiling other issues. Needing to do patch releases one after the other, etc. The same as when we changed the ignore_case=True in the FileCopier, which after several fixes still have some open issues. And it was "right" to fix it, but not worth the effort.

I would say that this case is similar. It is better to wait until Conan 2.0 to resolve these ambiguities (I haven't seen users reporting this), and if we want to implement now the feature request that the title is suggesting: Exclude a package when building all from sources, that might still be possible introducing the --build=!pattern, or --build-exclude=pattern or something like that.

@memsharded memsharded self-assigned this Feb 11, 2021
@uilianries
Copy link
Member Author

uilianries commented Feb 11, 2021

Indeed. Let's keep it for 2.0.

While --build=!pattern is more intuitive, our current argparse usage does not allow it. If we pass --build --build=anything, the argparse returns ['anything'], it discards --build because is None. However, if we pass --build= --build=anything, argparse returns ['anything', ''], which fits for --build=!anything.

So to avoid this kind of confusion, I would prefer a new argument, as you mentioned --build-exclude.

UPDATE:

Thinking again, we can accept --build=!foo too, it doesn't matter. If we pass --build --build=!foo or --build --build-exclude=foo, argparse will ignore --build.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries uilianries marked this pull request as ready for review February 11, 2021 18:11
@@ -58,6 +58,11 @@ def __call__(self, parser, namespace, values, option_strings=None): # @UnusedVa
dest.extend(values)
except ValueError:
dest.append(values)
# --build --build=foo == ["*", "foo"]
elif hasattr(namespace, "build"):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it, we won't be able to know if --build has been passed together with another --build=foo or --build=!foo. Argparse excludes None entries. As '*' is considered build all from sources and --build has the same usage, I don't think it will break Conan contract.

Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
Signed-off-by: Uilian Ries <uilianries@gmail.com>
elif hasattr(namespace, "build") and isinstance(namespace.build, list):
if len(namespace.build) == 0 or \
any(str(it).startswith("!") for it in namespace.build):
dest.append("*")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a exclude pattern shouldn't be tied to building from source everything. You could have --build=mypkgs* --build=!mypkgs_special and it should build all packages starting with mypkgs but not the excluded one.

Please try to avoid as much as possible to particularize a generic thing as Extender with custom logic from one of its consumer, it is an anti-pattern and should be used only in case of high necessity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think this might still be changing behavior, and this will probably bring issues, better to avoid it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default, when we pass --build --build=!foo, our parse only returns !foo, so doesn't matters whether --build is listed as command argument or not. This new conditional, only accepts --build=! when --build is present too. Or, we can create an implicit rule, like if you passed only --build=!foo, it means build all from sources, except for foo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer it to be explicit, and move in the direction of Conan 2.0 CLI than adding it magically. So we could document that --build=!xxx requires other patterns to be positively defined, even if --build=*. Better than customize the Extender class with specific attribute logic (in other command --build could mean a different thing (actually it does in conan build --build command, and this is a risk of failure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-build=!xxx requires other patterns to be positively defined

by contrary, we don't need to define an extra argument, without this workaround. If you agree, I'll remove it and keep only --build=!xxx, which is enough for us.

@@ -44,9 +46,25 @@ def __init__(self, params, output):

if self.never and (self.outdated or self.missing or self.patterns or self.cascade):
raise ConanException("--build=never not compatible with other options")
self._unused_patterns = list(self.patterns)
self._excluded_patterns = [it for it in list(self.patterns) if it.startswith("!")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once the exclude_patterns has been computed, we don't need the ! anymore. It seems it would be better to clean it directly while computing self._excluded_patterns

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea was printing to user that a package could be wrong named. If we don't keep that list, we can't provide such information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? I don't understand. We can keep that list, but in any case the ! is syntactic sugar, lets call it --build-exclude and that will remove all ambiguities.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see! Indeed if we use a new argument instead of !, we don't need to parse the value. Okay, if we will accept --build-exclude, I prefer accepting only it and excluding ! to avoid any ambiguous behavior.

fnmatch.fnmatchcase(repr(ref.copy_clear_rev()), pattern) or
fnmatch.fnmatchcase(repr(ref), pattern))

for pattern in self.patterns:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this use self._excluded_patterns? Seems a bit confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a possibility indeed, but the idea here is comparing if a pattern matches with excluded pattern, if so, exclude it from exclude patterns list and return False to say: Don't build this package from sources. All remaining patterns in excluded list will be printed to say: These packages don't match to a pattern to excluded.

conans/test/functional/graph/test_graph_build_mode.py Outdated Show resolved Hide resolved
conans/test/functional/graph/test_graph_build_mode.py Outdated Show resolved Hide resolved
@uilianries
Copy link
Member Author

@memsharded I'm not going to say that we traded 6 for half a dozen, but add a new argument for --build-exclude and forwarding it, inflicted more files than before. Well, at least we didn't touch arg parser.

We still need exclude_pattern list, because only when checking "force", the package reference is available for validation. Take a look and tell me if you see a point which can be simplified. Honestly, I don't like to forward and forward a parameter just for a simple feature.

@memsharded
Copy link
Member

@memsharded I'm not going to say that we traded 6 for half a dozen, but add a new argument for --build-exclude and forwarding it, inflicted more files than before. Well, at least we didn't touch arg parser.

We still need exclude_pattern list, because only when checking "force", the package reference is available for validation. Take a look and tell me if you see a point which can be simplified. Honestly, I don't like to forward and forward a parameter just for a simple feature.

Yes, I don't love this either... too much for such a feature. Need to re-think this one.

memsharded and others added 2 commits April 20, 2021 15:15
Signed-off-by: Uilian Ries <uilianries@gmail.com>
@uilianries
Copy link
Member Author

@memsharded Good to go

@memsharded memsharded added this to the 1.36 milestone Apr 20, 2021
@memsharded memsharded merged commit 4c89ed0 into conan-io:develop Apr 20, 2021
memsharded added a commit to memsharded/conan that referenced this pull request Apr 20, 2021
* Build all package when --build= is listed

If Conan install uses --build= it means all packages
must be built. If another package is listed too, it doesn't matter.
e.g conan install foo/0.1@ --build= --build=bar, where bar is
dependency. As --build= listed, all packages will be built from sources

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Argument value None is a valid entry

When passing --build= or --build they should be
the same, but actually they are considered different.
--build= means "", --build means None

However it's not intuitive if you are building a package:
conan install <ref> --build OR
conan install <ref> --build=

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build parameter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Revert last 3 commits

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate graph build mode

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Add test for ignore build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Accept skipped build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Fix empty argument validation

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build message

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* transform namespace.build in a list

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Improve build argument filter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Update --build help description

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* conan-io#8442 Replace --build=! by --build-exclude

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* fix broken arg

* same behavior as --build --build=pattern

* more test

* conan-io#8442 Add --build=! command help

Signed-off-by: Uilian Ries <uilianries@gmail.com>

Co-authored-by: James <james@conan.io>
czoido added a commit that referenced this pull request Apr 21, 2021
* wip

* wip

* working

* remove dead unittest

* CMake() needs CMakeToolchain

* fix tests

* minor refactor get_generator_platform

* Return 'deprecated' attribute in 'conan inspect' command (#8832)

* Return 'deprecated' attribute in 'conan inspect' command

* fix tests

* Exclude a package when building all from sources (#8483)

* Build all package when --build= is listed

If Conan install uses --build= it means all packages
must be built. If another package is listed too, it doesn't matter.
e.g conan install foo/0.1@ --build= --build=bar, where bar is
dependency. As --build= listed, all packages will be built from sources

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Argument value None is a valid entry

When passing --build= or --build they should be
the same, but actually they are considered different.
--build= means "", --build means None

However it's not intuitive if you are building a package:
conan install <ref> --build OR
conan install <ref> --build=

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build parameter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Revert last 3 commits

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate graph build mode

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Add test for ignore build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Accept skipped build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Fix empty argument validation

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build message

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* transform namespace.build in a list

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Improve build argument filter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Update --build help description

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* #8442 Replace --build=! by --build-exclude

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* fix broken arg

* same behavior as --build --build=pattern

* more test

* #8442 Add --build=! command help

Signed-off-by: Uilian Ries <uilianries@gmail.com>

Co-authored-by: James <james@conan.io>

* fix develop

* Check Artifactory service context for conan_build_info --v2 (#8826)

* check service context

* add unittests

* fix import

* use json file!

Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
memsharded added a commit to memsharded/conan that referenced this pull request Apr 22, 2021
* wip

* wip

* working

* remove dead unittest

* CMake() needs CMakeToolchain

* fix tests

* minor refactor get_generator_platform

* Return 'deprecated' attribute in 'conan inspect' command (conan-io#8832)

* Return 'deprecated' attribute in 'conan inspect' command

* fix tests

* Exclude a package when building all from sources (conan-io#8483)

* Build all package when --build= is listed

If Conan install uses --build= it means all packages
must be built. If another package is listed too, it doesn't matter.
e.g conan install foo/0.1@ --build= --build=bar, where bar is
dependency. As --build= listed, all packages will be built from sources

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Argument value None is a valid entry

When passing --build= or --build they should be
the same, but actually they are considered different.
--build= means "", --build means None

However it's not intuitive if you are building a package:
conan install <ref> --build OR
conan install <ref> --build=

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build parameter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Revert last 3 commits

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate graph build mode

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Add test for ignore build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Accept skipped build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Fix empty argument validation

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build message

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* transform namespace.build in a list

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Improve build argument filter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Update --build help description

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* conan-io#8442 Replace --build=! by --build-exclude

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* fix broken arg

* same behavior as --build --build=pattern

* more test

* conan-io#8442 Add --build=! command help

Signed-off-by: Uilian Ries <uilianries@gmail.com>

Co-authored-by: James <james@conan.io>

* fix develop

* Check Artifactory service context for conan_build_info --v2 (conan-io#8826)

* check service context

* add unittests

* fix import

* use json file!

Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
czoido added a commit that referenced this pull request Apr 26, 2021
* removing gcc, visual, and msbuild old generators

* removing more deprecated stuff

* fixing more tests

* fix export manifests test

* removing more legacy

* fixing tests

* fix test (#8840)

* Feature/cmake vs toolsets (#8815)

* wip

* wip

* working

* remove dead unittest

* CMake() needs CMakeToolchain

* fix tests

* minor refactor get_generator_platform

* Return 'deprecated' attribute in 'conan inspect' command (#8832)

* Return 'deprecated' attribute in 'conan inspect' command

* fix tests

* Exclude a package when building all from sources (#8483)

* Build all package when --build= is listed

If Conan install uses --build= it means all packages
must be built. If another package is listed too, it doesn't matter.
e.g conan install foo/0.1@ --build= --build=bar, where bar is
dependency. As --build= listed, all packages will be built from sources

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Argument value None is a valid entry

When passing --build= or --build they should be
the same, but actually they are considered different.
--build= means "", --build means None

However it's not intuitive if you are building a package:
conan install <ref> --build OR
conan install <ref> --build=

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build parameter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Revert last 3 commits

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate graph build mode

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Add test for ignore build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Accept skipped build package

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Fix empty argument validation

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Validate build message

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* transform namespace.build in a list

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Improve build argument filter

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* Update --build help description

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* #8442 Replace --build=! by --build-exclude

Signed-off-by: Uilian Ries <uilianries@gmail.com>

* fix broken arg

* same behavior as --build --build=pattern

* more test

* #8442 Add --build=! command help

Signed-off-by: Uilian Ries <uilianries@gmail.com>

Co-authored-by: James <james@conan.io>

* fix develop

* Check Artifactory service context for conan_build_info --v2 (#8826)

* check service context

* add unittests

* fix import

* use json file!

Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>

* fix new environment files with spaces in path (#8847)

* fixed win tests

Co-authored-by: Carlos Zoido <mrgalleta@gmail.com>
Co-authored-by: Javier G. Sogo <jgsogo@gmail.com>
Co-authored-by: Uilian Ries <uilianries@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[question] Is there a way to skip specific packages from build when calling install --build?
2 participants