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

[autoconf] conan v2 #12896

Merged
merged 45 commits into from
Nov 8, 2022
Merged

Conversation

jellespijker
Copy link
Contributor

Specify library name and version: autoconf/x.x.x

I took the liberty of defining a conf value: tools.autoconf:autoconf, tools.autoconf:autoreconf, tools.autoconf:autoheader, tools.autoconf:autom4te which are paths to the binary m4 executable. I don't know if tools is a reserved namespace but it maid sense in my mind. Let me know if that should change.

Based on this template:
https://github.com/conan-io/conan-center-index/tree/master/docs/package_templates/autotools_package

Contribute to CURA-8831


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the conan-center hook activated.

j.spijker@ultimaker.com and others added 26 commits September 1, 2022 16:58
Note: test_package can't use the new Autools generator
due to conan-io/conan#11975
KB-H013 states that everything needs to be installed under bin for tools.
This also means that the `PATH` for the actual bin location `bin/bin` needs
to be added to the `build_env`
subsystem isn't known at this time
No need to actually compile something with this tool
we simply need to know if it runs.

The win_bash for the run needs to be set explicitly
due to the conf not working properly in the test_package
see conan-io/conan#11975
This allows us to use the msys2 bash without using the conf_info
in the test_package. Which is currently problematic due to:
conan-io/conan#11975

Contributes to CURA-9574
Contributes to CURA-8831
The ducktyping seems to affect the usage
of the Autotools in the main recipe as well

Contributes to CURA-8831
Contributes to CURA-8831
Contributes to CURA-8831
The subsystem isn't known when consuming projects request the info

Contributes to CURA-8831
Using some duck typing and inheritance to work around
GH issue conan-io/conan#11980

Contributes to CURA-9595
@conan-center-bot

This comment has been minimized.

prince-chrismc
prince-chrismc previously approved these changes Oct 31, 2022
Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Matches what was discussed 👍

Unless there's a blocker I think fine tuning in the next PR is good :)

jwillikers
jwillikers previously approved these changes Oct 31, 2022
Co-authored-by: Chris Mc <prince.chrismc@gmail.com>
Comment on lines +33 to +34
if self._settings_build.os == "Windows" and not self.conf.get("tools.microsoft.bash:path", default=False, check_type=bool):
return # autoconf needs a bash if there isn't a bash no need to build
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be merged in this state where windows is not tested, needs conan 1.53.0 in c3i

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need 1.53? 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

conan-io/conan#12095. So currently test is skipped on Windows, not good.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jellespijker please remove this condition, now that conan 1.53.0 is dsployed, tools.microsoft.bash:path should be found in test package

Comment on lines +49 to +53
if can_run(self):
ext = ".exe" if self.settings.os == "Windows" else ""
test_cmd = unix_path(self, path.join(self.build_folder, f"test_package{ext}"))

self.run(test_cmd, env="conanbuild")
Copy link
Contributor

Choose a reason for hiding this comment

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

not need to protect with can_run, since it's executed in build scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be run scope? Seems odd to be using build scope in a "can the test run" scenario 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

yes it should be run scope

@conan-center-bot

This comment has been minimized.

jwillikers
jwillikers previously approved these changes Oct 31, 2022
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 17 (f76bdfe71b67d059c6162a4d0c64e67e7c14ce94):

  • autoconf/2.71@:
    All packages built successfully! (All logs)

  • autoconf/2.69@:
    All packages built successfully! (All logs)

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 5b8511e into conan-io:master Nov 8, 2022
@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 8, 2022

As I said in #12896 (comment), Windows is not tested, so I don't understand why it's approved. Actually it seems to break windows builds relying on autoreconf: #14110 (comment)

Where does it come from? tools.unix_path has been removed to env vars defined package_info(), so it breaks any Windows build calling autoconf.

@SpaceIm SpaceIm mentioned this pull request Nov 9, 2022
4 tasks
@SSE4
Copy link
Contributor

SSE4 commented Nov 9, 2022

As I said in #12896 (comment), Windows is not tested, so I don't understand why it's approved. Actually it seems to break windows builds relying on autoreconf: #14110 (comment)

Where does it come from? tools.unix_path has been removed to env vars defined package_info(), so it breaks any Windows build calling autoconf.

so, should we revert?

@SpaceIm
Copy link
Contributor

SpaceIm commented Nov 9, 2022

At least still convert all paths in package_info() with legacy conans.tools.unix_path

@hassec
Copy link

hassec commented Nov 21, 2022

@jellespijker did by any chance test that this recipe also runs from within msys2 ?

I've tried a bunch of different profiles to try to get

conan install -r conancenter autoconf/2.71@ -pr:b=default -pr:h=default --build=autoconf

to work but have had zero success with that.

@jellespijker
Copy link
Contributor Author

these changes were instigated specifically for use because of the need to run it in msys2.

At the moment we're not using the chain of dependencies (in which autoconf is a part with) in production but it should work.

we planned on using it with this recipe https://github.com/Ultimaker/conan-ultimaker-index/blob/CURA-8831_pyqt6_recipe/recipes/pyqt6/conanfile.py using the following configuration https://github.com/Ultimaker/conan-config/tree/CURA-8831_pyqt6_recipe

which should work on Mac, Linux and Windows.

I'm gonna try if broke on msys2 somewhere this week and come back to you. But I suggest that if it is indeed broken for you that you create a bug report, so the CCI maintainers can process it in their normal workflow

@hassec
Copy link

hassec commented Nov 22, 2022

@jellespijker Thanks for your prompt reply 😊 I've created an issue -> #14375

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.