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 2.0 compatibility #14925

Merged
merged 14 commits into from
Feb 15, 2023

Conversation

System-Arch
Copy link
Contributor

@System-Arch System-Arch commented Dec 24, 2022

Specify library name and version: autoconf/2.71

This PR addresses some legacy code that was left in the recipe to address an issue in Conan 1.x (conan-io/conan#12499) that is purported to be fixed in Conan 1.55. However, subsequent discussions with @memsharded have led to the discovery that this issue can be more cleanly addressed simply by eliminating the use of unix_path() in package_info. See conan-io/conan#12785 for more details.


@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@jcar87
Copy link
Contributor

jcar87 commented Jan 6, 2023

Hi @System-Arch, thanks for taking the time to migrate this recipe to support Conan 2! Very well done :)

From what I can see, the newer integrations and the way shell scripts are called, would make the use of tools_legacy.unix_path redundant, as conan already converts the paths automatically before when invoking scripts within the msys2 shell. (As a side note: using tools_legacy.unix_path and leaving Conan to guess the subsystem may not work well everywhere). So all in all, a good change!

Unfortunately, I believe that merging the PR as it is would probably break any current consumer recipes that are not using the new integrations, so we're in a bit of a catch-22 situation. This does not only apply to recipes that depend on autoconf directly, but also the ones that depend on it transitively via automake.

There's good news and bad news. The good: only two environment variables defined here are used publicly (AUTORECONF and AUTOHEADER), and since they point to executables that are already in the PATH (which is handled specially and converted correctly for bash on Windows), they could be replaced to simply point to autoreconf and autoheader or similar, and leave shell to find it in the path. I've tested this and I think this would retain compatibility for current consumers. This recipe already proves this works - as it is setting to fallback to finding m4 in /usr/bin/env m4 if the M4 environment variable does not work (and the m4 recipe does not use unix_path in package info). This would also extend to the variables defined in package info that point to scripts in the bin folder, which are not used directly by recipes, and only referenced by automake or autoconf scripts.

The bad news: AC_MACRODIR and AUTOM4TE_PERLLIBDIR - these are used only internally by the autoconf scripts themselves. A typical installation of Autoconf would fallback to looking into the prefix provided at build/package time, but since that is not relocatable, the recipe has logic to ensure those env vars are defined so that when consumers run the autoconf scripts, it points to the right location inside their local cache.

Unfortunately without this unix_path conversion, when the perl scripts run inside the msys2 shell on the consumer's side, if this env var is left with the Windows path format, the scripts will fail trying to read the contents of the directory as the path is not in the right format.
The conclusions of conan-io/conan#12785 are correct, unix_path should not be used in package_info() and should be resolved in the consumer's side. The caveat here is that AC_MACRODIR and AUTOM4TE_PERLLIBDIR are not being evaluated by the consumers directly so there isn't really anything to change on the consumer side.

I'm investigating a workaround to see if this can be made relocatable without relying on performing path conversions in package_info() - which, even as the recipe currently is, is actually broken on some systems anyway.

@jcar87
Copy link
Contributor

jcar87 commented Jan 6, 2023

Having investigated this a little further, I believe we can refactor the way this recipe does "relocatability" in a way that doesn't rely on environment variables. With the advantage that we can retain the environment variables to avoid breaking existing downstream consumers, while at the same time avoiding any unix <-> windows path conversions.

Currently chasing a potential bug in Conan 2.0 beta 7 - one that I suspect only manifests itself in macOS when using 2 profiles Figured out what was missing :)

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@CLAassistant
Copy link

CLAassistant commented Jan 7, 2023

CLA assistant check
All committers have signed the CLA.

@ghost
Copy link

ghost commented Jan 9, 2023

I detected other pull requests that are modifying autoconf/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

@System-Arch
Copy link
Contributor Author

Hi @jcar87,

Thanks for your help with moving this effort forward via #15179 (as well as you sign-off on my CMake changes). I was wondering if you could look at the remaining changes in this PR and let me know if they are okay? Thanks

prince-chrismc
prince-chrismc previously approved these changes Jan 28, 2023
Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution @System-Arch !! I've left a comment as I think we need to either fix the conditional for the patch on windows, or amend the package_id logic

recipes/autoconf/all/conanfile.py Outdated Show resolved Hide resolved
recipes/autoconf/all/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@System-Arch
Copy link
Contributor Author

Hi @jcar87,
I believe I have implemented the changes you requested. I've marked the issues as resolved but I'm not sure if there is some way in GitHub to show that the request has been addressed. Anyway let me know if you have an additional suggestions or concerns. Thanks,

@System-Arch System-Arch requested review from jcar87 and prince-chrismc and removed request for jcar87 and prince-chrismc February 7, 2023 00:08
prince-chrismc
prince-chrismc previously approved these changes Feb 8, 2023
jwillikers
jwillikers previously approved these changes Feb 10, 2023
StellaSmith
StellaSmith previously approved these changes Feb 13, 2023
Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

hi @System-Arch, thanks so much for this PR. This actually fixes the ability to build on Windows :D

I believe the tool_requires needs to stay as well - in fact, this could cause some problems on macOS.
The autom4te scripts generated during the build of this package has flags that are compatible with the m4 that was available during build time (build time of the autoconf package).

if the tool_requires on m4 is removed, on macOS the build will use the m4 provided by Xcode, and the generated autom4ke file will be slightly different than it would otherwise be if it depended on Conan Center's m4. I wouldn't risk this :D

@System-Arch
Copy link
Contributor Author

hi @System-Arch, thanks so much for this PR. This actually fixes the ability to build on Windows :D

I believe the tool_requires needs to stay as well - in fact, this could cause some problems on macOS. The autom4te scripts generated during the build of this package has flags that are compatible with the m4 that was available during build time (build time of the autoconf package).

if the tool_requires on m4 is removed, on macOS the build will use the m4 provided by Xcode, and the generated autom4ke file will be slightly different than it would otherwise be if it depended on Conan Center's m4. I wouldn't risk this :D

Hi @jcar87,
Thanks for the insights. I had assumed that if something was listed as a requirement, it was automatically also a build requirement, but it makes sense that they are distinct sets. I've added the tool_requires back in. Thanks

P.S. That said, with both Coann 1.x and Conan 2.0, I see m4 get installed during the build phase even if it is only specified as a requirement, so I admit that I am still unclear on the behavior vis-à-vis a requirement and a build_requirement when it comes to building. I wonder if this is a Conan bug and that it should not be installing requirements at build time unless they are explicitly declared via tool_requires in the build_requriements method(). CC'ing @memsharded for any additional insights.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 14 (628a11e864ecf5016fbba7f241809163ff3fa44b):

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

Conan v2 pipeline (informative, not required for merge) ❌

Note: Conan v2 builds are informative and they are not required for the PR to be merged.

The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future.

See details:

Failure in build 14 (628a11e864ecf5016fbba7f241809163ff3fa44b):

  • autoconf/2.71@:
    CI failed to create some packages (All logs)

    Logs for packageID da39a3ee5e6b4b0d3255bfef95601890afd80709:
    [settings]
    arch=x86_64
    build_type=Release
    compiler=apple-clang
    compiler.libcxx=libc++
    compiler.version=13
    os=Macos
    
    [...]
    arch=x86_64
    build_type=Release
    compiler=apple-clang
    compiler.libcxx=libc++
    compiler.version=13
    os=Macos
    [conf]
    tools.system.package_manager:mode=install
    tools.system.package_manager:sudo=True
    tools.apple:sdk_path=/Applications/conan/xcode/13.0/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.3.sdk
    
    
    -------- test_package: Computing dependency graph --------
    m4/1.4.19: Not found in local cache, looking in remotes...
    m4/1.4.19: Checking remote: conan-center-v2
    m4/1.4.19: Trying with 'conan-center-v2'...
    Downloading conanmanifest.txt
    Downloading conanfile.py
    Downloading conan_export.tgz
    Decompressing conan_export.tgz
    m4/1.4.19: Downloaded recipe revision 7689d2cae05587d0efb12261ddc4124d
    Graph root
        autoconf/2.71 (test package): /Users/jenkins/w/prod-v2/BuildSingleReference/conan-center-index/recipes/autoconf/all/test_package/conanfile.py
    Build requirements
        autoconf/2.71#f4e2bd681d49b4b80761aa587bde94d5 - Cache
        m4/1.4.19#7689d2cae05587d0efb12261ddc4124d - Cache
    
    -------- test_package: Computing necessary packages --------
    
    -------- Computing necessary packages --------
    Build requirements
        autoconf/2.71#f4e2bd681d49b4b80761aa587bde94d5:da39a3ee5e6b4b0d3255bfef95601890afd80709#5c53a519d70fdb79d3cd4145e9f0355f - Download (c3i_PR-v2-14925)
        m4/1.4.19#7689d2cae05587d0efb12261ddc4124d:9ac8640923e5284645f8852ef8ba335654f4020e - Skip
    
    -------- test_package: Installing packages --------
    
    -------- Installing (downloading, building) binaries... --------
    ERROR: Missing binary: m4/1.4.19:9ac8640923e5284645f8852ef8ba335654f4020e
    
    m4/1.4.19: WARN: Can't find a 'm4/1.4.19' package binary '9ac8640923e5284645f8852ef8ba335654f4020e' for the configuration:
    [settings]
    arch=x86_64
    build_type=Release
    os=Macos
    
    ERROR: Missing prebuilt package for 'm4/1.4.19'
    Use 'conan list packages m4/1.4.19 --format=html -r=remote > table.html' and open the table.html file to see available packages
    Or try to build locally from sources with '--build=m4/1.4.19'
    
    More Info at 'https://docs.conan.io/en/latest/faq/troubleshooting.html#error-missing-prebuilt-package'
    

Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability.

Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

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

LGTM! :) Thanks so much for fixing this - in particular, the issue when building this on Windows, which I've experienced first hard and it must not have been an easy one :)

@jcar87
Copy link
Contributor

jcar87 commented Feb 15, 2023

hi @System-Arch, thanks so much for this PR. This actually fixes the ability to build on Windows :D
I believe the tool_requires needs to stay as well - in fact, this could cause some problems on macOS. The autom4te scripts generated during the build of this package has flags that are compatible with the m4 that was available during build time (build time of the autoconf package).
if the tool_requires on m4 is removed, on macOS the build will use the m4 provided by Xcode, and the generated autom4ke file will be slightly different than it would otherwise be if it depended on Conan Center's m4. I wouldn't risk this :D

Hi @jcar87, Thanks for the insights. I had assumed that if something was listed as a requirement, it was automatically also a build requirement, but it makes sense that they are distinct sets. I've added the tool_requires back in. Thanks

P.S. That said, with both Coann 1.x and Conan 2.0, I see m4 get installed during the build phase even if it is only specified as a requirement, so I admit that I am still unclear on the behavior vis-à-vis a requirement and a build_requirement when it comes to building. I wonder if this is a Conan bug and that it should not be installing requirements at build time unless they are explicitly declared via tool_requires in the build_requriements method(). CC'ing @memsharded for any additional insights.

It is indeed possible to list something as a requirement (that also gets propagated downstream), and have it available at build time - which works great when building targetting the same OS/architecture, but may backfire when cross-building. tool_requires don't propagate downstream and only apply when building from source, while requires propagate to consumers. When cross-building, tool_requires typically apply to executables - and Conan will know to bring them with the correct OS/architecture.

@System-Arch
Copy link
Contributor Author

It is indeed possible to list something as a requirement (that also gets propagated downstream), and have it available at build time - which works great when building targetting the same OS/architecture, but may backfire when cross-building. tool_requires don't propagate downstream and only apply when building from source, while requires propagate to consumers. When cross-building, tool_requires typically apply to executables - and Conan will know to bring them with the correct OS/architecture.

Hi @jcar87, Thanks for the additional insights. Conan does indeed have its subtle nuances. Thanks

@conan-center-bot conan-center-bot merged commit 363bc44 into conan-io:master Feb 15, 2023
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.

8 participants