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

Remove packages orphaned by FairMQ change #4870

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

TimoWilken
Copy link
Contributor

@TimoWilken TimoWilken commented Mar 2, 2023

This removes asiofi, asio and ofi since they were only used by FairMQ prior to #4869.

This PR also cleans up some leftovers from #4869, including one now-unused variable. @ktf, There was a comment in the FairMQ recipe warning not to disable the -DBUILD_EXAMPLES, but since you changed that, can the comment go too?

In addition, port the change to FairMQ from #4860 to batch the O2 rebuild with the one from this PR and/or #4869.

@TimoWilken TimoWilken requested a review from ktf March 2, 2023 11:12
@TimoWilken TimoWilken requested a review from a team as a code owner March 2, 2023 11:12
@dennisklein
Copy link
Contributor

There was a comment in the FairMQ recipe warning not to disable the -DBUILD_EXAMPLES, but since you changed that, can the comment go too?

Just a headsup to avoid double efforts: I will make a PR within the next days with a more general cleanup of the fairmq.sh recipe.

@TimoWilken
Copy link
Contributor Author

Hi @dennisklein, great, thanks!

If you're already changing the FairMQ recipe, perhaps you'll also find https://github.com/TimoWilken/alidistlint useful -- we're in the process of introducing this as a code checker for alidist recipes. It finds common issues in shell scripts and alidist recipe metadata.

@dennisklein
Copy link
Contributor

dennisklein commented Mar 2, 2023

@TimoWilken @ktf Maybe a quick style question:

We have lots of CMake configure options of this form ${ZEROMQ_ROOT:+-DZeroMQ_ROOT=$ZEROMQ_ROOT}, do you prefer to have it explicitely written out in the cmake configure command line? alibuild started out by using capitalized $<PACKAGE>_ROOT env variables while CMake then later introduced support for case-sensitive <Package>_ROOT env and CMake variables. In principle, one could drop explicit CMake config lines in the form above since FairMQ supports the capitalized env variables set by alibuild - I would personally appreciate the reduced boilerplate, but I am not sure about your latest (de facto) policies here?

@TimoWilken
Copy link
Contributor Author

@dennisklein, I'm not sure that "our" CMake actually uses the $<PACKAGE>_ROOT env vars by default -- at least the way I understand cmake --help-policy CMP0074 is that the env vars are ignored by default, and we don't configure the policy otherwise as far as I know.

@dennisklein
Copy link
Contributor

Maybe some more details: FairMQ's CMake uses find_package2 which is implemented here: https://github.com/FairRootGroup/FairCMakeModules/blob/c64d6b486febe4555a011c636a7284267ae29297/src/modules/FairFindPackage2.cmake.in#L157-L160. As you can see, find_package2 looks up env variables of the capitalized form $<PACKAGE>_ROOT. I support this (basically since forever) exactly for alibuild which sets those capitalized env variables when running the recipes.

@TimoWilken
Copy link
Contributor Author

Ah, fair enough! In that case, I don't mind either way. I don't know of an ALICE policy on this, I don't think it's come up before...

@TimoWilken TimoWilken force-pushed the fairmq-remove-orphans branch from 6f1cb76 to f4e0c29 Compare March 7, 2023 17:40
@TimoWilken
Copy link
Contributor Author

Rebased and removed cleanups obsoleted by #4882. Now, this PR just removes the unused asio, asiofi and ofi recipes.

@davidrohr davidrohr merged commit 8115d5b into alisw:master Mar 8, 2023
@TimoWilken TimoWilken deleted the fairmq-remove-orphans branch March 29, 2023 11:49
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