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
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion conans/client/command.py
Expand Up @@ -41,7 +41,7 @@ class Extender(argparse.Action):
settings = ['cucumber:true']
"""
def __call__(self, parser, namespace, values, option_strings=None): # @UnusedVariable
# Need None here incase `argparse.SUPPRESS` was supplied for `dest`
# Need None here in case `argparse.SUPPRESS` was supplied for `dest`
dest = getattr(namespace, self.dest, None)
if not hasattr(dest, 'extend') or dest == self.default:
dest = []
Expand All @@ -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") 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.



class OnceArgument(argparse.Action):
Expand Down Expand Up @@ -2233,6 +2238,9 @@ def settings_args(machine, short_suffix="", long_suffix=""):
source.
--build=[pattern] Build packages from source whose package reference matches the pattern. The
pattern uses 'fnmatch' style wildcards.
--build=![pattern] Exclude packages to be built from source whose package reference matches the
pattern. The pattern uses 'fnmatch' style wildcards.


Default behavior: If you omit the '--build' option, the 'build_policy' attribute in conanfile.py
will be used if it exists, otherwise the behavior is like '--build={}'.
Expand Down
28 changes: 23 additions & 5 deletions conans/client/graph/build_mode.py
Expand Up @@ -10,6 +10,7 @@ class BuildMode(object):
=> False if user wrote "never"
=> True if user wrote "missing"
=> "outdated" if user wrote "--build outdated"
=> ["!foo"] means exclude when building all from sources
"""
def __init__(self, params, output):
self._out = output
Expand All @@ -19,6 +20,7 @@ def __init__(self, params, output):
self.cascade = False
self.patterns = []
self._unused_patterns = []
self._excluded_patterns = []
self.all = False
if params is None:
return
Expand All @@ -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.

self._unused_patterns = [it for it in list(self.patterns)
if it not in self._excluded_patterns]

def forced(self, conan_file, ref, with_deps_to_build=False):
def pattern_match(pattern):
return (fnmatch.fnmatchcase(ref.name, pattern) or
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.

if pattern.startswith("!") and pattern_match(pattern[1:]):
try:
self._excluded_patterns.remove(pattern)
except ValueError:
pass
conan_file.output.info("Excluded build from source")
return False

if self.never:
return False
if self.all:
Expand All @@ -62,9 +80,7 @@ def forced(self, conan_file, ref, with_deps_to_build=False):

# Patterns to match, if package matches pattern, build is forced
for pattern in self.patterns:
if (fnmatch.fnmatchcase(ref.name, pattern) or
fnmatch.fnmatchcase(repr(ref.copy_clear_rev()), pattern) or
fnmatch.fnmatchcase(repr(ref), pattern)):
if pattern_match(pattern):
try:
self._unused_patterns.remove(pattern)
except ValueError:
Expand All @@ -83,4 +99,6 @@ def allowed(self, conan_file):

def report_matches(self):
for pattern in self._unused_patterns:
self._out.error("No package matching '%s' pattern" % pattern)
self._out.error("No package matching '%s' pattern found to be built." % pattern)
for pattern in self._excluded_patterns:
self._out.error("No package matching '%s' pattern found to be excluded." % pattern)
167 changes: 167 additions & 0 deletions conans/test/functional/graph/test_graph_build_mode.py
@@ -0,0 +1,167 @@
import pytest
import operator
from conans.test.assets.genconanfile import GenConanfile
from conans.test.utils.tools import TestClient


class fakeop:
memsharded marked this conversation as resolved.
Show resolved Hide resolved
def __init__(self, function):
self.function = function
def __ror__(self, other):
return fakeop(lambda x, self=self, other=other: self.function(other, x))
def __or__(self, other):
return self.function(other)
def __rlshift__(self, other):
return fakeop(lambda x, self=self, other=other: self.function(other, x))
def __rshift__(self, other):
return self.function(other)
def __call__(self, value1, value2):
return self.function(value1, value2)


@pytest.fixture(scope="module")
def build_all():
""" Build a simple graph to test --build option

foobar <- bar <- foo
<--------|

All packages are built from sources to keep a cache.
:return: TestClient instance
"""
client = TestClient()
client.save({"conanfile.py": GenConanfile().with_setting("build_type")})
client.run("export . foo/1.0@user/testing")
client.save({"conanfile.py": GenConanfile().with_require("foo/1.0@user/testing")
.with_setting("build_type")})
client.run("export . bar/1.0@user/testing")
client.save({"conanfile.py": GenConanfile().with_require("foo/1.0@user/testing")
.with_require("bar/1.0@user/testing")
.with_setting("build_type")})
client.run("export . foobar/1.0@user/testing")
client.run("install foobar/1.0@user/testing --build")

return client


def test_install_build_single(build_all):
""" When only --build=<ref> is passed, only <ref> must be built
"""
build_all.run("install foobar/1.0@user/testing --build=foo")
uilianries marked this conversation as resolved.
Show resolved Hide resolved

assert "bar/1.0@user/testing:7839863d5a059fc6579f28026763e1021268c55e - Cache" in build_all.out
assert "foo/1.0@user/testing:4024617540c4f240a6a5e8911b0de9ef38a11a72 - Build" in build_all.out
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - Cache" in build_all.out
assert "foo/1.0@user/testing: Forced build from source" in build_all.out
assert "bar/1.0@user/testing: Forced build from source" not in build_all.out
assert "foobar/1.0@user/testing: Forced build from source" not in build_all.out
assert "No package matching" not in build_all.out


def test_install_build_double(build_all):
""" When both --build=<ref1> and --build=<ref2> are passed, only both should be built
"""
build_all.run("install foobar/1.0@user/testing --build=foo --build=bar")
uilianries marked this conversation as resolved.
Show resolved Hide resolved

assert "bar/1.0@user/testing:7839863d5a059fc6579f28026763e1021268c55e - Build" in build_all.out
assert "foo/1.0@user/testing:4024617540c4f240a6a5e8911b0de9ef38a11a72 - Build" in build_all.out
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - Cache" in build_all.out
assert "foo/1.0@user/testing: Forced build from source" in build_all.out
assert "bar/1.0@user/testing: Forced build from source" in build_all.out
assert "foobar/1.0@user/testing: Forced build from source" not in build_all.out
assert "No package matching" not in build_all.out


@pytest.mark.parametrize("build_arg,mode", [("--build", "Build"),
("--build=", "Cache"),
("--build=*", "Build")])
def test_install_build_only(build_arg, mode, build_all):
""" When only --build is passed, all packages must be built from sources
When only --build= is passed, it's considered an error
When only --build=* is passed, all packages must be built from sources
"""
build_all.run("install foobar/1.0@user/testing {}".format(build_arg))

assert "bar/1.0@user/testing:7839863d5a059fc6579f28026763e1021268c55e - {}".format(mode) in build_all.out
assert "foo/1.0@user/testing:4024617540c4f240a6a5e8911b0de9ef38a11a72 - {}".format(mode) in build_all.out
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - {}".format(mode) in build_all.out
opin = fakeop(lambda x, y: x in y) if mode == "Build" else fakeop(lambda x, y: x not in y)
assert "foo/1.0@user/testing: Forced build from source" |opin| build_all.out
memsharded marked this conversation as resolved.
Show resolved Hide resolved
assert "bar/1.0@user/testing: Forced build from source" |opin| build_all.out
assert "foobar/1.0@user/testing: Forced build from source" |opin| build_all.out
assert not "No package matching" |opin| build_all.out


@pytest.mark.parametrize("build_arg,bar,foo,foobar", [("--build", "Cache", "Build", "Cache"),
("--build=", "Cache", "Build", "Cache"),
("--build=*", "Build", "Build", "Build")])
def test_install_build_all_with_single(build_arg, bar, foo, foobar, build_all):
""" When --build is passed with another package, only the package must be built from sources.
When --build= is passed with another package, only the package must be built from sources.
When --build=* is passed with another package, all packages must be built from sources.
"""
build_all.run("install foobar/1.0@user/testing --build=foo {}".format(build_arg))

assert "bar/1.0@user/testing:7839863d5a059fc6579f28026763e1021268c55e - {}".format(bar) in build_all.out
assert "foo/1.0@user/testing:4024617540c4f240a6a5e8911b0de9ef38a11a72 - {}".format(foo) in build_all.out
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - {}".format(foobar) in build_all.out
for ref, mode in [("foo", foo), ("bar", bar), ("foobar", foobar)]:
opin = fakeop(lambda x, y: x in y) if mode == "Build" else fakeop(lambda x, y: x not in y)
assert "{}/1.0@user/testing: Forced build from source".format(ref) |opin| build_all.out


@pytest.mark.parametrize("build_arg,bar,foo,foobar", [("--build", "Build", "Cache", "Build"),
("--build=", "Cache", "Cache", "Cache"),
("--build=*", "Build", "Cache", "Build")])
def test_install_build_all_with_single_skip(build_arg, bar, foo, foobar, build_all):
""" When --build is passed with a skipped package, not all packages must be built from sources.
When --build= is passed with another package, only the package must be built from sources.
When --build=* is passed with another package, not all packages must be built from sources.

The arguments order matter, that's why we need to run twice.
"""
for argument in ["--build=!foo {}".format(build_arg), "{} --build=!foo".format(build_arg)]:
build_all.run("install foobar/1.0@user/testing {}".format(argument))

assert "bar/1.0@user/testing:7839863d5a059fc6579f28026763e1021268c55e - {}".format(bar) in build_all.out
assert "foo/1.0@user/testing:4024617540c4f240a6a5e8911b0de9ef38a11a72 - {}".format(foo) in build_all.out
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - {}".format(foobar) in build_all.out
for ref, mode in [("foo", foo), ("bar", bar), ("foobar", foobar)]:
opin = fakeop(lambda x, y: x in y) if mode == "Build" else fakeop(lambda x, y: x not in y)
assert "{}/1.0@user/testing: Forced build from source".format(ref) |opin| build_all.out


@pytest.mark.parametrize("build_arg,bar,foo,foobar", [("--build", "Cache", "Cache", "Build"),
("--build=", "Cache", "Cache", "Cache"),
("--build=*", "Cache", "Cache", "Build")])
def test_install_build_all_with_double_skip(build_arg, bar, foo, foobar, build_all):
""" When --build is passed with a skipped package, not all packages must be built from sources.
When --build= is passed with another package, only the package must be built from sources.
When --build=* is passed with another package, not all packages must be built from sources.

The arguments order matter, that's why we need to run twice.
"""
for argument in ["--build=!foo --build=!bar {}".format(build_arg),
"{} --build=!foo --build=!bar".format(build_arg)]:
build_all.run("install foobar/1.0@user/testing {}".format(argument))

assert "bar/1.0@user/testing:7839863d5a059fc6579f28026763e1021268c55e - {}".format(bar) in build_all.out
assert "foo/1.0@user/testing:4024617540c4f240a6a5e8911b0de9ef38a11a72 - {}".format(foo) in build_all.out
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - {}".format(foobar) in build_all.out


def test_report_matches(build_all):
""" When a wrong reference is passed to be build, an error message should be shown
"""
build_all.run("install foobar/1.0@user/testing --build --build=baz")
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - Build" in build_all.out
assert "No package matching 'baz' pattern found to be built."

build_all.run("install foobar/1.0@user/testing --build --build=!baz")
assert "No package matching 'baz' pattern found to be excluded."
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - Build" in build_all.out

build_all.run("install foobar/1.0@user/testing --build --build=!baz --build=blah")
assert "No package matching 'blah' pattern found to be built."
assert "No package matching 'baz' pattern found to be excluded."
assert "foobar/1.0@user/testing:89636fbae346e3983af2dd63f2c5246505e74be7 - Build" in build_all.out