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

More flexibility in Autotools tools to override arguments and avoid all default arguments #11284

Merged
merged 30 commits into from May 27, 2022

Conversation

czoido
Copy link
Contributor

@czoido czoido commented May 17, 2022

Changelog: Fix: Use DESTDIR argument in make install step instead of using the --prefix in configure.
Changelog: Feature: More flexibility in Autotools tools to override arguments and avoid all default arguments for make, autoreconf and configure.
Docs: conan-io/docs#2562

Some notes:

  • As suggested in this issue, we are using now DESTDIR argument for make install.
  • As a side effect, we can remove the second configure in install and put the build_script_folder argument back in the configure method instead of having it in the constructor.
  • Also, now that we don't have to use the package_folder for the --prefix argument, I have removed the default_configure_install_args attribute and moved the default install arguments to the toolchain.

Closes: #11265
Closes: #11264

@czoido czoido added this to the 1.49 milestone May 17, 2022
@czoido czoido changed the title Do not add --enable-shared/--enable-static/--without-pic/--with-pic configure flags to the Autotools build helper by default Do not add some configure flags to the Autotools build helper and improve install step May 18, 2022
@czoido czoido marked this pull request as draft May 18, 2022 11:47
# because there's no package_folder until the package step
self.configure()
self.make(target="install")
self.make(target="install", args=["DESTDIR={}".format(self._conanfile.package_folder)])

Choose a reason for hiding this comment

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

If one sets default_configure_install_args to False, they may not want the DESTDIR either.

Having used the tools in anger for a bit, having extra arguments added under the hood seems to be an anti pattern and hides the magic.

Personally, I would much prefer if autotoolstoolchain auto populated the autoreconf_args/configure_args/make_args and I could modify them as I see fit before generation.

Usage would then be:

    def generate(self) -> None:
        at = AutotoolsToolchain(self)
        print(at.autoreconf_args) # has the `--force` and `--install` arguments in
        print(at.configure_args) # has the `--prefix` and `--shared` arguments in
        print(at.make_args) # has `DESTDIR`  and `-j` in it
        # I know _exactly_ will be passed to `autoreconf`, `configure` and `make`

I'd even argue adding --enable-checking=fatal would be a good default that users can easily remove with:

        at.configure_args = [a for a in at.configure_args if a.startswith("--enable-checking")]

Then, if I have a really wonky package I can customise the arguments entirely. There are so many odd autotools packages in the wild.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for the review @mattyclarkson, I think something in that direction would make sense, I'll add some changes to see how it looks

@czoido czoido marked this pull request as ready for review May 23, 2022 10:44
conans/assets/templates/new_v2_autotools.py Outdated Show resolved Hide resolved
@czoido czoido changed the title Do not add some configure flags to the Autotools build helper and improve install step More flexibility in Autotools tools to override arguments and avoid all default arguments May 23, 2022
@memsharded
Copy link
Member

@mattyclarkson this is looking better for me now, but it would be great if you could have a look to the changes and see if they align with your expectations.

@memsharded memsharded merged commit d41822f into conan-io:develop May 27, 2022
@memsharded
Copy link
Member

Merged, because we need to start branching 1.49, but please @mattyclarkson if you have any feedback, it would be great to have it. Thanks!

czoido added a commit to czoido/conan that referenced this pull request Jun 2, 2022
…ll default arguments (conan-io#11284)

* add test

* wip

* wip

* wip

* mark test

* change name

* minor changes

* wip

* build_script_folder in configure

* put configure bacl

* wip

* revert

* change name

* remove second configure

* revert

* fix test

* make it more configurable

* wip

* update test

* add shared flags by default

* update method

* refactor and remove default_configure_install_args

* update test

* add cpp to mock

* review

* raise in mock

* update test

* remove with-pic
czoido added a commit to czoido/conan that referenced this pull request Jun 2, 2022
…ll default arguments (conan-io#11284)

* add test

* wip

* wip

* wip

* mark test

* change name

* minor changes

* wip

* build_script_folder in configure

* put configure bacl

* wip

* revert

* change name

* remove second configure

* revert

* fix test

* make it more configurable

* wip

* update test

* add shared flags by default

* update method

* refactor and remove default_configure_install_args

* update test

* add cpp to mock

* review

* raise in mock

* update test

* remove with-pic
@mattyclarkson
Copy link

Merged, because we need to start branching 1.49, but please @mattyclarkson if you have any feedback, it would be great to have it. Thanks!

Yo, sorry, I was AFK for a while taking a break. This looks great 🎉 Thanks for the changes. We will roll to the newer versions and try it out. If we have any extra feedback, I'll make sure to create an issue but this looks in great shape for 2.0.

args = args or ["--force", "--install"]
command.extend(args)
command = join_arguments(command)
args = args or []

Choose a reason for hiding this comment

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

Whilst getting familiar with the Conan code base, I noticed this Python anti-pattern.

The code base could simplify the code flow by moving towards immutable tuples rather than mutable lists.

# Python3 with expandable iterables syntax
def autoreconf(self, args=("--force", "--install")):
    command = join_arguments(("autoreconf", self._autoreconf_args, *args))
    with chdir(self, self._conanfile.source_folder):
        self._conanfile.run(command)

# Python2/3 with extending from iterables
def autoreconf(self, args=("--force", "--install")):
    command = ["autoreconf"]
    command.extend(self._autoreconf_args)
    command.extend(args)
    with chdir(self, self._conanfile.source_folder):
        self._conanfile.run(command)

The above is totally valid because the static binding of the tuple to the args is immutable it is not possible to change the bound value within the function. This is a common problem in code bases that use mutable bindings in argument defaults which we are avoiding by using None. i.e. the following would break terribly:

def autoreconf(self, args=["--force", "--install"]):
    args.append("foobar")  # everytime `autoreconf` is invoked `args` gets longer

In general, I personally use immutable values as much as possible to avoid unintended side effects. It's a shame that Python doesn't have records for immutable dictionaries.

Comment on lines +94 to 96
command = join_arguments(["autoreconf", self._autoreconf_args, args_to_string(args)])
with chdir(self, self._conanfile.source_folder):
self._conanfile.run(command)

Choose a reason for hiding this comment

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

It seems a common pattern in Conan that self.run needs to join arguments. It would be pretty neat if, run could accept an iterable and automatically join them:

    def run(self, command, output=True, cwd=None, win_bash=False, subsystem=None, msys_mingw=True,
            ignore_errors=False, run_environment=False, with_login=True, env="conanbuild"):
        if not isinstance(co```py
    def run(self, command, output=True, cwd=None, win_bash=False, subsystem=None, msys_mingw=True,
            ignore_errors=False, run_environment=False, with_login=True, env="conanbuild"):
        if not isinstance(command, str) and isinstance(command, abc.Iterable):
            command = join_arguments(command)
```mmand, str) and isinstance(command, abc.Iterable):
            command = join_arguments(command)

For Conan v2.0 where we can shed Python 2, I'd change that API to force keywords and allow passing multiple arguments:

    def run(self, *command, output=True, cwd=None, win_bash=False, subsystem=None, msys_mingw=True,
            ignore_errors=False, run_environment=False, with_login=True, env="conanbuild"):
        command = join_arguments(command)

Then it can be used more naturally:

self.run("autoreconf", "--force", "--install", env="conanrun")

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