-
Notifications
You must be signed in to change notification settings - Fork 19
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
[several] Ubuntu 24.04 support + various improvements #246
Conversation
76b5590
to
0ea64b6
Compare
f66560a
to
e83e26d
Compare
f1e627c
to
7224e24
Compare
2619ebc
to
5ba1255
Compare
Tests are passing! This PR is ready for review. |
Good stuff. May not be able to look at this until after UGM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. I just noticed a couple of things in the new module.
Also, would it be good to include some light instructions regarding the patch system? Maybe this is a common thing with which I am unfamiliar, but it seems like it would be good to leave the process for finding / applying patches for posterity. We can also create an issue for adding that to the README (similar to the directions about bumping build number versus bumping build version). Just a thought.
Good stuff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good. Please await another approval and I think we're good to # it.
A couple of follow-up questions:
-
Do we need to release new externals packages before merging the companion PRs?
-
Had a question about this from OP: "This PR introduces a python module distro_info.py which is likely to make its way over to the main irods repository soon-ish." How do we feel about maintaining a PyPi package? Is that too much? Just thinking ahead to the duplicate code question... (one here, one in main repo)
Neither of these questions are blockers for merging, but just figured I'd put them here.
Only the ones that are still drafts.
I don't think that's a solution. If everything goes to plan, it's going to be a development (and possibly runtime) dependency of irods. Therefore, if it's not part of the repository, it has to be installable from a package we can declare a dependency on. We can't declare dependencies on PyPi packages, which is why we have the special handling for pyodbc on centos 7. |
Did your testing involve all platforms supported by iRODS? My guess is yes due to the nature of the PR. What about plugins? If there turns out to be an issue caused by a patch, how do you imagine we address them? Have you considered that? |
I did not run the test suite on any other platforms, nor did I run any plugin test suites. I did, however, ensure that everything still built on all platforms.
Depending on the situation, more than likely one of the following:
A new externals package would need to be published with the updated patch set. |
And for cases where the patch is fixing a broken external, I imagine this would be a |
Generally speaking, yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're happy with changes, squash to taste. No pounds.
I wasn't planning on squashing any more than I already have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pound it.
#'d |
Addresses #89
Addresses #236
Addresses #237
Addresses #238
Addresses #239
Addresses #240
Addresses #241
Addresses #242
Addresses #243
Addresses #244
Addresses #245
Addresses #248
In service of irods/irods#7592
Companion PRs:
Notes:
In order to build clang 13 against GCC 13 and libstdc++ 13, some patching is needed that is too complex to handle with basic shell commands, so this PR introduces new patching functionality.
Additionally, a couple of the nlohmann-json tests fail to build. This is fixed upstream by patching the tests themselves. This is fixed in this PR by simply turning off the tests.
distro_info.py
which is likely to make its way over to the main irods repository soon-ish.