-
-
Notifications
You must be signed in to change notification settings - Fork 28
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
GCC 14.1, 13.3 & 12.4 #133
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@conda-forge/ctng-compilers, this is ready for review! Point releases for the maintenance branches should show up towards the end of May. |
Ah, we might want to fix #126 here already. |
@conda-forge-admin, please rerender |
Not sure what's up after #130, but I cannot rerender locally anymore.
Might be running into path length limitations on windows... At least the bot still can. :) |
…nda-forge-pinning 2024.05.13.13.50.55
We can carry two sets in |
@conda-forge/ctng-compilers, this should be ready for review. We might want to link the C++ chrono implementation to a tzdb we supply ourselves (with |
@conda-forge-admin, please rerender |
…nda-forge-pinning 2024.05.21.12.55.42
This now also contains 13.3 & the tzdb fix. PTAL @conda-forge/ctng-compilers :) |
Thanks. 13.3 build fails to apply the patches though. |
For tzdb, can you add a test? Binary prefix relocation usually don't work when the prefix is stored in a std::string. |
OK, if you prefer platform-specific implementations over environment variables, that's fine by me. 🤷 |
So I tried looking at this, and it's not trivial (from what I can see) to get the path to the library where the function lives. It'd be a bit easier to get the path of the calling executable, but that's useless because it has unknown relationship to Or could you tell me how you you envisioned getting a path at a stable/predictable depth in the environment file hierarchy from the C++ side? |
Thanks. I found the dladdr approach too, but obviously that won't work for windows, where we'd have to use |
…nda-forge-pinning 2024.06.20.18.47.46
So I implemented something for the relative path approach here, but obviously using I think it should be something like |
+ tz_dir += "/share/zoneinfo"; | ||
+ // std::string constructor deep-copies | ||
+ free(this_lib); | ||
+ return tz_dir.c_str(); |
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.
This pointer outlives the memory it points to when it is being returned.
We should return a pointer to statically allocated memory instead, I think.
Additionally, the allocations from this_lib
and tz_dir
are not really needed.
For this_lib
you can just continue to use resolved
instead.
And if you allocate buffer
with size PATH_MAX + sizeof(TO_PREFIX "/share/zoneinfo")
you can append the suffix directly to resolved
in buffer
's memory then.
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.
Thanks! For now, I'm just trying to get something working1. Improving the implementation can be done afterwards (even if it happens as part of this PR after all).
Footnotes
-
despite trying to keep the implementation minimal/simple, I remain strongly doubtful that the trade-off of all that complexity just for avoiding a corner³ case compared to the env var approach is beneficial. ↩
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.
Improving the implementation can be done afterwards (even if it happens as part of this PR after all).
This was just me seeing a use-after-free error, so wanted to make sure that we don't include it in lower level libs.
I remain strongly doubtful that the trade-off of all that complexity just for avoiding a corner case compared to the env var approach is beneficial.
<chrono>
use and in conjunction with hitting tzdb absence/differences is a corner case as of now, yes.
Not having CONDA_PREFIX
set or set to a different path is really common, though (e.g., think about conda-build
: we have a host
and build
environment -- only packages from one of them can use CONDA_PREFIX
at runtime).
There are plethora of ways things can go wrong with reliance on env vars (and with reliance on activation scripts even more so) -- it's too brittle for us to rely on in lower level libs.
(Wcan chat about why/where there are so many problems with that if things are unclear, if you want.)
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.
Not having
CONDA_PREFIX
set or set to a different path is really common, though (e.g., think aboutconda-build
: we have ahost
andbuild
environment -- only packages from one of them can useCONDA_PREFIX
at runtime).
It's easy to probe all of CONDA_PREFIX
resp. PREFIX
/ BUILD_PREFIX
(if CONDA_BUILD
is set), and even without anything, we'd find the system tzdb.
So a bug related to this requires: no valid env vars (which is not the case in any of our build infra or user envs), an out-of-date system tzdb (<-- user's fault/problem IMO), stumbling over anything that actually changed in the tzdb compared to an older system tzdb (almost nothing changes unless a country changes its laws about timezones), and using a timestamp just around the summer-/wintertime change. It's so extremely narrow a problem that I don't think the implementation effort for relative paths is anywhere near justified.
In any case, I'm trying to follow your and Isuru's preference. Help with the implementation is appreciated (so thanks for pointing out the bug) - I didn't expect it to be ready yet either, hence the debugging skips here. Right now I'm not even hitting the UAF due to the problems related to linking libdl
, so that's the first thing to tackle.
I'm going to go with this for now:
I'll put up another PR with the WIP patch, and we can work from there, without indefinitely blocking the version upgrade. |
Any objections to merging @isuruf? |
@@ -30,7 +30,7 @@ package: | |||
version: {{ version }} | |||
|
|||
source: | |||
- url: https://sourceware.org/pub/gcc/releases/gcc-{{ version }}/gcc-{{ version }}.tar.gz | |||
- url: https://ftp.gnu.org/gnu/gcc/gcc-{{ version }}/gcc-{{ version }}.tar.gz |
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.
FWIW, 12.4 wasn't available on the ftp server shortly after release, and the announcement made it seem like that won't happen (also my understanding of sourceware was that it's much closer to GCC than third-party). Just for context why I changed that.
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.
Ah, thanks. I didn't see the FSF affiliation.
@isuruf, AFAICT we have a bootstrapping problem here:
I guess the subdir confusion is coming from a missing |
Freshly released: 14.1, 13.3, 12.4