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

Correct encoding of make_program path for AutoTools + msys2 #15047

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

nmasseyKM
Copy link
Contributor

Changelog: (Bugfix): AutoTools Describe here your pull request (fixes #15046)

Autotools expects to be run in a unix-like environment. So the correct
encoding for paths to execute is unix-like.
@CLAassistant
Copy link

CLAassistant commented Nov 2, 2023

CLA assistant check
All committers have signed the CLA.

@memsharded memsharded self-assigned this Nov 3, 2023
@memsharded
Copy link
Member

Thanks very much for your PR!
I will try to reduce a bit the test scope, it is taking 1:30 in my Windows machine to run, I'll try to cover the case but reducing the time to run.

@memsharded
Copy link
Member

In 5b33028 I have reduced the test so it runs in a few seconds, and still covers the use case.

However, I am a bit concerned about the change:

  • At the moment the users can define the make_program via conf. They can specify there any value they want, with the syntax and "unix_path" they want.
  • It can be failing if users provide a path with \ backslash, but the solution is easy, provide a path with forward slash
  • If we force now to convert to unix_path, then in all scenarios it will be tried to be converted. If for some reason some users have a Make outside of the subsystem, it might fail. I know it is not that likely, but still possible that users are using a subsystem and still their toolchain, including make is not installed in that subsystem. For example, this is more likely for some use cases of Autotools with bare Makefiles, not really configure scripts. I know this was not the original intention of Autotools, but it happens.

Please let me know what you think.

@nmasseyKM
Copy link
Contributor Author

In 5b33028 I have reduced the test so it runs in a few seconds, and still covers the use case.

However, I am a bit concerned about the change:

  • At the moment the users can define the make_program via conf. They can specify there any value they want, with the syntax and "unix_path" they want.

They can also specify "bash:path" with the syntax they want (in system or "unix_path" syntax), and autotools works correctly. That's the expected behavior I am trying to replicate.

  • It can be failing if users provide a path with \ backslash, but the solution is easy, provide a path with forward slash

Agreed. If the user is providing the path manually, they can conform to a required syntax.

However, this path is being supplied programmatically in by make's conanfile.py as

        make = os.path.join(self.package_folder, "bin", "gnumake.exe" if self.settings.os == "Windows" else "make")
        self.conf_info.define("tools.gnu:make_program", make)

and it seems like that syntax should result in a valid path.

Of secondary consideration, trying to script this in windows tools (such as powershells Get-Command cmdlit) will also provide the paths in Windows native form.

  • If we force now to convert to unix_path, then in all scenarios it will be tried to be converted. If for some reason some users have a Make outside of the subsystem, it might fail.

You're right, I think 922cb85 is a cleaner approach.

I know it is not that likely, but still possible that users are using a subsystem and still their toolchain, including make is not installed in that subsystem. For example, this is more likely for some use cases of Autotools with bare Makefiles, not really configure scripts. I know this was not the original intention of Autotools, but it happens.

It's definitely possible to have a msys2 subsystem without make installed. That's how I discovered this issue. :-)
If that's the case, and "tools.gnu:make_program" is not set, then Autotools.make() is going to fail regardless of what we do to the default value: default behavior is to expand to a make which is alongside bash.

The user has the option of using their profile to configure the package they would like to provide "tools.gnu:make_program", which seems a sensible path since that configuration would be alongside the configuration of "bash:path" and "bash:subsystem".
Recipes could address this if they want, along the lines of

    def build_requirements(self):
        if self._settings_build.os == "Windows":
            self.win_bash = True
            if not self.conf.get("tools.microsoft.bash:path", check_type=str):
                self.tool_requires("msys2/cci.latest")
            else if not self.conf.get("tools.gnu:make_program", check_type=str):
                self.tool_requires("make/[>=4.0]")

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.

3 participants