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

[config] Re-order Mac M1 configuration #23594

Closed

Conversation

uilianries
Copy link
Member

This change puts Mac M1 to be listed first when building packages in ConanCenterIndexCI. It does not guarantee the result as builds are execute in parallel, so a test package could ask for package that's not available yet.

Besides this change, Conan team will check for internal options/fix in C3I Jenkins code.

Related to #23558


Signed-off-by: Uilian Ries <uilianries@gmail.com>
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.

under review

@jcar87
Copy link
Contributor

jcar87 commented Apr 17, 2024

having taken the decision to simplify the test_packages, I'm not sure this is worth the effort.

The test_packages should have never reached the level of complexity that some have reached. Guaranteeing a build order "would" fix the issue of test_packages failing when they require two package IDs of the same reference (that are built as part of the same PR), but it has a downside that it serialises what can otherwise be parallel.

Out of 1743 test_packages in the repository, only the following have a dual requirement against the tested reference:

libtool/all/test_package/conanfile.py
mbits-lngs/all/test_package/conanfile.py
mfast/all/test_package/conanfile.py
extra-cmake-modules/all/test_package/conanfile.py
grpc/all/test_package/conanfile.py
tree-gen/all/test_package/conanfile.py
flatcc/all/test_package/conanfile.py
btyacc/all/test_package/conanfile.py
protobuf-c/all/test_package/conanfile.py
qt/5.x.x/test_package/conanfile.py
qt/6.x.x/test_package/conanfile.py
wayland/all/test_package/conanfile.py
pkgconf/all/test_package/conanfile.py
cfgfile/all/test_package/conanfile.py
libtasn1/all/test_package/conanfile.py
ignition-cmake/all/test_package/conanfile.py
gsoap/all/test_package/conanfile.py
flex/all/test_package/conanfile.py

that's 18, in addition to protobuf and capnproto which have already been fixed. Guaranteeing a build order by serialising the creation of the binary packages is not worth it only to support these 18 cases vs the 1743 test packages we have currently. Personally I dont think most of the above are justified in having a complex test package, other than Qt.

I think we should refocus our efforts in ensuring that the very valid PRs against protobuf and the likes that are impacted by the issues can be swiftly merged, while in the background we continue our efforts to bring the test packages back in line with the original intention of the test packages.

If this PR doesn't really guarantee that the test_packages will succeed due to race conditions, I wouldn't bother too much.

@ericLemanissier
Copy link
Contributor

@jcar87 OTOH this change has no adverse effect, and allows easy progress on all the recipes you listed

@jcar87
Copy link
Contributor

jcar87 commented Apr 17, 2024

@jcar87 OTOH this change has no adverse effect, and allows easy progress on all the recipes you listed

All the recipes I've listed will have their test package refactored accordingly, should this become a problem (I think Qt is more complex and we will explore an alternative)

@ericLemanissier
Copy link
Contributor

I guess we don't use the same ruler to measure the height of low hanging fruits

@mayeut mayeut mentioned this pull request May 3, 2024
3 tasks
@uilianries
Copy link
Member Author

Closing as not planned.

@uilianries uilianries closed this May 24, 2024
@jcar87
Copy link
Contributor

jcar87 commented May 24, 2024

After further testing, this did not mitigate the issue.

@conan-io conan-io locked and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants