search

conda-forge / ctng-compilers-feedstock

A conda-smithy repository for ctng-compilers.
BSD 3-Clause "New" or "Revised" License
12 stars 25 forks source link

GCC 14.1, 13.3 & 12.4 #133

Closed h-vetinari closed 2 weeks ago

h-vetinari commented 2 months ago

Freshly released: 14.1, 13.3, 12.4

conda-forge-webservices[bot] commented 2 months ago

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 (recipe) and found it was in an excellent condition.

h-vetinari commented 2 months ago

@conda-forge/ctng-compilers, this is ready for review! Point releases for the maintenance branches should show up towards the end of May.

h-vetinari commented 2 months ago

Ah, we might want to fix #126 here already.

h-vetinari commented 2 months ago

@conda-forge-admin, please rerender

h-vetinari commented 2 months ago

Not sure what's up after #130, but I cannot rerender locally anymore.

  File "E:\miniforge\envs\builder\Lib\site-packages\conda_smithy\feedstock_io.py", line 55, in write_file
    with io.open(filename, "w", encoding="utf-8", newline="\n") as fh:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\[xxx]\\dev\\conda-forge\\ctng-compilers-feedstock\\.ci_support\\linux_aarch64_cross_target_platformlinux-64cross_target_stdlibsysrootcross_target_stdlib_version2.12gcc_maj_ver11gcc_version11.4.0old_tripletx86_64-conda_cos6-linux-gnutripletx86_64-conda-linux-gnu.yaml'

Might be running into path length limitations on windows...

At least the bot still can. :)

h-vetinari commented 2 months ago

@isuruf, unsurprisingly, the mingw patches from #130 fail against GCC 14. What would you like to do here - carry two sets of patches, or build mingw only against GCC 13 (or only against 14)?

isuruf commented 2 months ago

We can carry two sets in recipe/patches/mingw/13 and recipe/patches/mingw/14

h-vetinari commented 2 months ago

@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 tzdata; see #126), but for now this is a vanilla upgrade.

h-vetinari commented 1 month ago

@conda-forge-admin, please rerender

h-vetinari commented 1 month ago

This now also contains 13.3 & the tzdb fix. PTAL @conda-forge/ctng-compilers :)

isuruf commented 1 month ago

Thanks. 13.3 build fails to apply the patches though.

isuruf commented 1 month ago

For tzdb, can you add a test? Binary prefix relocation usually don't work when the prefix is stored in a std::string.

h-vetinari commented 1 month ago

Thanks. 13.3 build fails to apply the patches though.

Yeah, one of the mingw patches appeared in 13.3 and needs to be dropped.

For tzdb, can you add a test? Binary prefix relocation usually don't work when the prefix is stored in a std::string.

How do you imagine that test to look like? I imagine it'll be a bit hard to prove 100% that it gets taken from the specified location, since we also have a copy of tzdata in the sysroot: https://github.com/conda-forge/linux-sysroot-feedstock/issues/62.

I can compile an example that fetches something from the tzdb, is that enough?

As noted in #126, we could in principle also define zoneinfo_dir_override (see here), though I don't know how that would work in practice (where that weak symbol needs to be defined/implemented/shipped). If we do that, we could implement that function so that it always prefers $CONDA_PREFIX/share/zoneinfo (and loads the env var at runtime), so we'd avoid the binary relocation issues.

conda-forge-webservices[bot] commented 1 month ago

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

h-vetinari commented 1 month ago

Linter needs https://github.com/conda-forge/conda-smithy/pull/1946 (+ a smithy release)

h-vetinari commented 1 month ago

For tzdb, can you add a test? Binary prefix relocation usually don't work when the prefix is stored in a std::string.

This now has a custom zoneinfo_dir_override that loads from the respective environment variables dynamically, so no relocation necessary to catch the right tzdb-location. I've also adapted some upstream tests to double-check that things work.

PTAL @conda-forge/ctng-compilers

isuruf commented 1 month ago

Thanks for adding tests and also doing the change. I'm not a fan of using env variables here. It's the only thing that depends on env variables and if we could avoid it, that should be better. (One idea is to use relative paths)

Question: where does this code end up in? libstdc++.so or g++ ?

h-vetinari commented 1 month ago

I'm not a fan of using env variables here. It's the only thing that depends on env variables and if we could avoid it, that should be better. (One idea is to use relative paths)

I think using zoneinfo_dir_override is how GCC envisions these overrides. I've kept it as a weak symbol so that users of the conda-forge packages can still override it themselves.

I find $PREFIX/share/zoneinfo easier to follow and less brittle than relative paths, but ultimately I don't care that much how we get this done. It's early days for <chrono> support, so I think already that it works is a bonus, much less that we can iterate on the technical details if we want.

Overall though, I think the env var approach is the most portable/uniform, because we'll also need this for libcxx (and I also recently added this in orc to solve the same problem for windows).

Question: where does this code end up in? libstdc++.so or g++ ?

With "code" you mean the zoneinfo_dir_override implementation? I'm not very familiar with weak symbols, but AFAIU it's still part of the library.

h-vetinari commented 1 month ago

It's the only thing that depends on env variables and if we could avoid it, that should be better. (One idea is to use relative paths)

I think this is the first time that the C++ stdlib depends on external resources, so we don't really have something to compare to here. Regarding relative paths, those won't work (well) across the board AFAICT, because the headers are at different depth relative to $PREFIX between unix & windows (due to extra Library/ on windows) - which isn't so much an issue for libstdcxx, but will be needed for tzdb-support on windows in other places (e.g. libcxx, orc). IMO it would be good to have a consistent approach.

Given that these environment variables are always there in conda-forge (and we rely on them in the compiler activation already), I don't see the downsides of relying on them for the tzdb as well. In any case, we still have at least a year before GCC 14 becomes the default compiler version in conda-forge, and we could use that time to refine the setup (also seeing how things go with libcxx 19 etc.).

If you're still dead set against env vars, can we merge this PR without any tzdb/tzdata bits, just the vanilla version upgrade?

h-vetinari commented 1 month ago

@isuruf, please pick one:

  1. move forward with 13.3, park 14.1 and figure out tzdb stuff later
  2. use env vars for tzdb support (at least for now, can be refined later)
  3. explain alternative implementation to env vars
    • if relative paths, respond to shortcomings/compatibility concerns mentioned above
  4. other approach, with explanation
conda-forge-webservices[bot] commented 1 month ago

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 (recipe) and found it was in an excellent condition.

isuruf commented 1 month ago

@conda-forge-admin, rerender

isuruf commented 1 month ago

So, this code ends up in libstdc++.so which is in almost all envs. Using CONDA_PREFIX/PREFIX env variable will lead to hard to debug issues when an executable used while the conda environment is activated or not. Most of the important conda packages work without environment activation, and I'd like to keep it that way. Another issue is with statically linked executables like micromamba which will now have different behaviour based on what CONDA_PREFIX is.

Let's remove the patch and merge and figure out tzdb stuff later.

h-vetinari commented 1 month ago

Most of the important conda packages work without environment activation, and I'd like to keep it that way.

That's fair, though they would still keep working, because by default libstdcxx will just go look into /usr/share/zoneinfo, and we've kept that fallback for unactivated environments. I've added a test to this effect.

Another issue is with statically linked executables like micromamba which will now have different behaviour based on what CONDA_PREFIX is.

Same here.

Using CONDA_PREFIX/PREFIX env variable will lead to hard to debug issues when an executable used while the conda environment is activated or not.

I'm highly doubtful of this surfacing anywhere. A tzdb will always be found on a unix system, so the only issues then would have to be caused by divergences between the system tzdb and what we have in tzdata (completely aside from the fact that C++20 <chrono> isn't used anywhere yet).

Let's remove the patch and merge and figure out tzdb stuff later.

GCC 12.4 will be released on June 20th, so I propose to wait until then in any case. If you are willing to consider the above arguments (and tests), I'd prefer to move forward as-is, but happy to drop the patch if that's the cost of moving forward.

isuruf commented 1 month ago

A tzdb will always be found on a unix system, so the only issues then would have to be caused by divergences between the system tzdb and what we have in tzdata (completely aside from the fact that C++20 isn't used anywhere yet).

I didn't mean that tzdb will not be found. I meant tzdb divergence in all of my arguments.

(completely aside from the fact that C++20 isn't used anywhere yet).

I don't understand this. It's not used anywhere yet, but it will be in the future.

h-vetinari commented 4 weeks ago

I meant tzdb divergence in all of my arguments.

Can you outline a case where that would ever matter? To me this is completely hypothetical. For example, if a timezone name isn't found the code will error clearly. Yes, it's conceivable to get shifts by 1h (or a missing leap second) under extremely narrow circumstances, but I don't see how that falls under "hard to debug", because the obvious source is the tzdb (aside from the fact that C++20 provides chrono::remote_version, which allows authors to sanity-check the tzdb version in-code).

It's not used anywhere yet, but it will be in the future.

My point is that usage is at zero and there will be a very gradual adoption curve, which would allow us to respond to any eventual issues. The bug surface you're pointing out is IMO so narrow (someone has an outdated system tzdb, wants to use our libstdcxx builds without any activation, has <chrono> code, and manages to find a tzdb divergence to trip over) that I doubt it would be hit in practice.

I also wouldn't be arguing here if I saw a better solution; but we don't have constant depth of the C++ stdlib relative to $PREFIX across platforms, so IMO relative paths are either broken in some places, or more fragile than env vars once we need to start probing several directory levels.

isuruf commented 4 weeks ago

I don't see how that falls under "hard to debug", because the obvious source is the tzdb

That might be obvious to you right now, but it will almost always be hard to debug for the user.

but we don't have constant depth of the C++ stdlib relative to $PREFIX across platforms

It's always in $PREFIX/lib/libstdc++.so.6

h-vetinari commented 4 weeks ago

It's always in $PREFIX/lib/libstdc++.so.6

What I mean is that on windows it's under

%PREFIX%/Library/bin/libstdc++-6.dll
         ^^^^^^^^ 
         extra level compared to $PREFIX/lib/libstdc++.so.6

so ../../share/zoneinfo will not work where - in some way - it's "most" important to get this right, because we don't have the fallback to /usr/share/zoneinfo there. The same problem applies to libc++. Ideally, I'd like to have a solution that is capable of fixing this issue for both unix & windows uniformly.

Of course, you can say that we don't need to care about non-default C++ stdlibs on windows, but then we probably shouldn't publish them (rather than leaving <chrono> broken)? And even then there's things like orc, which want the tzdb despite not using C++20 yet (and where the env var approach was good enough as a fallback for upstream to merge).

isuruf commented 4 weeks ago

What I mean is that on windows it's under

That's a simple #ifdef _WIN32 .

h-vetinari commented 4 weeks ago

OK, if you prefer platform-specific implementations over environment variables, that's fine by me. 🤷

h-vetinari commented 3 weeks ago

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 $PREFIX. C++20's std::source_location also doesn't help. The best-looking solution I found is the small whereami library (particularly wai_getModulePath), but copying/vendoring that seems like major overkill for something that's solved in a handful of lines when using environment variables...

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?

isuruf commented 3 weeks ago

See https://github.com/gcc-mirror/gcc/blob/9a76db24e044c8058497051a652cca4228cbc8e9/libsanitizer/sanitizer_common/sanitizer_dl.cpp#L25-L35 for an example.

h-vetinari commented 3 weeks ago

See https://github.com/gcc-mirror/gcc/blob/9a76db24e044c8058497051a652cca4228cbc8e9/libsanitizer/sanitizer_common/sanitizer_dl.cpp#L25-L35 for an example.

Thanks. I found the dladdr approach too, but obviously that won't work for windows, where we'd have to use GetModuleFileNameW or similar.

h-vetinari commented 3 weeks ago

So I implemented something for the relative path approach here, but obviously using dladdr on linux means we need to link libstd++ to libdl. For now I've just hacked this with LDFLAGS (and even that I cannot get to work once something links to libstd++.so itself), but what I'd like to do is use the equivalent of CMakes target_link_libraries for libstd++.so itself.

I think it should be something like AC_LIB_LINKFLAGS, but the makefile(s) in GCC are just indecipherable for me (though I really tried to find where the shared lib gets defined...). Any ideas how/where to do this?

h-vetinari commented 3 weeks ago

I'm going to go with this for now:

@isuruf: Let's remove the patch and merge and figure out tzdb stuff later.

I'll put up another PR with the WIP patch, and we can work from there, without indefinitely blocking the version upgrade.

h-vetinari commented 3 weeks ago

Any objections to merging @isuruf?

h-vetinari commented 2 weeks ago

@isuruf, AFAICT we have a bootstrapping problem here:

Could not solve for environment specs
The following package could not be installed
└─ gcc_impl_win-64 13.3.0.*  is not installable because it requires
   └─ libgcc-devel_win-64 13.3.0 h5200ebd_100, which requires
      └─ __win, which is missing on the system.

libgcc-devel_win-64 is being built in that same job, but gcc (the output where things fail) wasn't part of the first mingw builds, so by the time it was enabled (https://github.com/conda-forge/ctng-compilers-feedstock/commit/e60f041730ef7292d9dfca6563e48df405485962) we already had some libgcc builds to work with.

I guess the subdir confusion is coming from a missing build: environment (even if it's empty, like it is for gcc_impl).