Skip to content

apr: allow recipes which support cross_building to consume exising apr binaries#16782

Merged
conan-center-bot merged 3 commits into
conan-io:masterfrom
puetzk:apr-cross
May 17, 2023
Merged

apr: allow recipes which support cross_building to consume exising apr binaries#16782
conan-center-bot merged 3 commits into
conan-io:masterfrom
puetzk:apr-cross

Conversation

@puetzk
Copy link
Copy Markdown
Contributor

@puetzk puetzk commented Mar 29, 2023

Since conancenter only provides x86_64 binaries on windows, it's frequently for necessary for an overall build which uses --settings:host arch=x86 --build missing to produce 32-bit requires from conancenter recipes to need --settings:build=x86-64 in order to satisfy tool_requires.

There are cross_building scenarios apr can't support, but that one would be fine, the current recipe validation is just stricter than it needed to be.


puetzk added 2 commits March 29, 2023 18:17
As discussed in conan-io#6622 (comment)
conan 1.38.0 and up have the new conan.tools.build.cross_building which
compares build vs host settings, without the fallback on os_build/arch_build
that the old tools.cross_compiling has.

The recipe was converted in conan-io#14797 to use conan.tools.build.cross_building
and required_conan_version = ">=1.53.0", making the workaround is obsolete.
From all the discussion I found in conan-io#6622, the issue which motivated a
"doesn't support cross-build yet" is the use of is AC_TRY_RUN checks
in ./configure. That's not an issue when consuming existing binaries,
so this check belongs in validate_build (since conan 1.51.0).

It also doesn't apply to MSVC which uses CMake rather than autoconf;
There's no use of try_compile or try_run in the CMake project.
@conan-center-bot

This comment has been minimized.

@puetzk
Copy link
Copy Markdown
Contributor Author

puetzk commented Mar 30, 2023

Do not raise ConanInvalidConfiguration from build() method. Please, move that same check to validate() method.

The ConanInvalidConfiguration is not in build(), it's in validate_build(). This is what the docs recommend: https://docs.conan.io/2/reference/conanfile/methods/validate_build.html#validate-build and I find several other examples in conan-center-index:

Any idea what the bot's objecting to, or a more detailed log?

@puetzk
Copy link
Copy Markdown
Contributor Author

puetzk commented Mar 30, 2023

Found the more detailed logs, and he failures came back from "[required] apr/1.7.0@ macOS, Clang (M1/arm64)", specifically https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/16782/1-configs/macos-m1-clang/apr/1.7.0//summary.json

Which seems to have worked as intended. To pick on one specific example, it was trying to cross-compile from x86_64 to armv8
https://c3i.jfrog.io/c3i/misc/logs/pr/16782/1-configs/macos-m1-clang/apr/1.7.0//172c221ccbd15188b78586021201c31d32ead0b1-build.txt

and was told it can't:

Cross-build from 'Macos:x86_64' to 'Macos:armv8'
Installing (downloading, building) binaries...
ERROR: There are invalid packages (packages that cannot exist for this configuration):
apr/1.7.0: Cannot build for this configuration: apr recipe doesn't support cross-build yet due to runtime checks in autoconf

This wasn't in build() - unlike the successes, there's no "apr/1.7.0: Calling build()" message printed like there is for ones that could be built, because vaidate_build() failed first.

These same configurations show INVALID_CONFIGURATION in e.g. #14797 https://c3i.jfrog.io/c3i/misc/summary.html?json=https://c3i.jfrog.io/c3i/misc/logs/pr/14797/5/apr/1.7.0//summary.json. As expected, given that apr indeed cannot cross-compile.

So it seems like the difference has got to be something that changed about c3i?

@puetzk
Copy link
Copy Markdown
Contributor Author

puetzk commented Mar 30, 2023

Oh duh, it's probably just picking up on the fact that armv8-Macos-clang binaries would now be allowed to exist, even though c3i still can't build them, since it only has a cross-compiler agent and not a native apple silicon one). So now it tried, and failed.

So apparently whatever implements this check:

Do not raise ConanInvalidConfiguration from build() method. Please, move that same check to validate() method.

Must have evaluated the graph ahead of time and pruned INVALID_CONFIGURATION, but either it ignored graph.Node.cant_build, or did that preliminary evaluation of the graph (deciding which package_ids to attempt building) without specifying --profile:build (and thus the recipe didn't know that it was going to be a cross-compile). Since c3i is closed-source I'm not sure how to distinguish those, or how to fix it...

@puetzk
Copy link
Copy Markdown
Contributor Author

puetzk commented Mar 30, 2023

Nevermind, the old code (in validate(), knew it was cross-compiling, so --profile:host was set. So it must be that c3i doesn't prune based on cant_build (i.e. invalid_build/invalid_build_reason if it's using the conan info json output...)

@ghost
Copy link
Copy Markdown

ghost commented Apr 1, 2023

I detected other pull requests that are modifying apr/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.

@ghost ghost mentioned this pull request Apr 30, 2023
1 task
@stale
Copy link
Copy Markdown

stale Bot commented May 1, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@AbrilRBS
Copy link
Copy Markdown
Member

Raising InvalidConfiguration from valida_build, retriggering the PR now, thanks a lot for your patience :)

@conan-center-bot
Copy link
Copy Markdown
Contributor

Conan v1 pipeline ✔️

All green in build 4 (5a0df8404430914311570688d52af1c0472c0016):

  • apr/1.7.0@:
    All packages built successfully! (All logs)

Conan v2 pipeline ✔️

Note: Conan v2 builds may be required once they are on the v2 ready list

All green in build 2 (5a0df8404430914311570688d52af1c0472c0016):

  • apr/1.7.0@:
    All packages built successfully! (All logs)

@AbrilRBS
Copy link
Copy Markdown
Member

@puetzk thanks a lot for your contribution and patience while we implemented the necessary logic in the CI to handle cases like this, sorry that we did not communicate properly that work was underway to have this PR merged. Also, thanks for such a detailed investigation and description of the problem, it has helped a lot when reviewing the change itself!

@conan-center-bot conan-center-bot merged commit fcfba50 into conan-io:master May 17, 2023
@puetzk puetzk deleted the apr-cross branch May 30, 2023 16:37
pezy pushed a commit to pezy/conan-center-index that referenced this pull request Jun 1, 2023
…onsume exising apr binaries

* apr: Remove settings_build workaround for tools.cross_building

As discussed in conan-io#6622 (comment)
conan 1.38.0 and up have the new conan.tools.build.cross_building which
compares build vs host settings, without the fallback on os_build/arch_build
that the old tools.cross_compiling has.

The recipe was converted in conan-io#14797 to use conan.tools.build.cross_building
and required_conan_version = ">=1.53.0", making the workaround is obsolete.

* apr: Loosen the cross-building restriction

From all the discussion I found in conan-io#6622, the issue which motivated a
"doesn't support cross-build yet" is the use of is AC_TRY_RUN checks
in ./configure. That's not an issue when consuming existing binaries,
so this check belongs in validate_build (since conan 1.51.0).

It also doesn't apply to MSVC which uses CMake rather than autoconf;
There's no use of try_compile or try_run in the CMake project.

---------

Co-authored-by: Rubén Rincón Blanco <rubenrb@jfrog.com>
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.

4 participants