Polish specs for non-cross, tweak config for style

timsnyder commented 2 years ago

Checklist

[X] Used a personal fork of the feedstock to propose changes
[X] Bumped the build number (if the version is unchanged)
[X] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[X] Ensured the license file is being packaged.

fixes #89 fixes #90

change manpath to $PREFIX/man so that they are just found when the environment is activated
set the pkgversion to indicate conda-forge with our buildnumber PKG_BUILD_STRING is set to placeholder at configure time or I would have used that. Maybe the hash isn't finalized yet?
set bugurl to ctng-compilers-feedstock newissue link
guard rpath related flags with %{!static: } because I think they will break static builds. Also add -rpath-link, -disable_new_dtags and a -L
Add -idirafter to specs for cc1_options and cpp_options to put $PREFIX/include at the end of the builtin search path like the system packages do for /usr/local/include
Create a copy of the 'builtin' specs called builtin.specs that don't have any conda-forge munging done to them. Enables users to pass -specs=builtin.specs to reset all of our spec changes if needed.
Also, only do the spec patching when build_platform is target_platform. Perhaps I really mean to guard this with %(cross_compile) somehow because build_platform == target_platform is cross during toolchain build but we want the specs to be be operational when the toolchain is deployed and not cross compiling

conda-forge-linter commented 2 years 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.

timsnyder commented 2 years ago

I probably need to add some tests for the behavior of the specs but I wanted to build more of the combinations in the matrix so that I can try them out and determine if they are working the way I want in the cross builds and cross builds of non-cross toolchains.

timsnyder commented 2 years ago

If the selectors for the tests are correct, I need to change the patching of the specs to happen when cross_target_platform == target_platform. I'll think through this some more after I've had some sleep.

timsnyder commented 2 years ago

@isuruf, what do you think of this implementation? The more I think about this, the more it makes sense to me that the specs are only done when cross_target_platform == target_platform but I always create builtin.specs so as a user of the toolchain, you can always provide -specs=builtin.specs and be confident that you're getting what shipped with gcc.

isuruf commented 2 years ago

I'm not a fan of using -idirafter which means headers included in $PREFIX cannot override the standard headers using #include_next because -I $PREFIX/include -idirafter $PREFIX/include is the same as -idirafter $PREFIX/include.

How about using %include_noerr to include an extra spec file that is then packaged in a package conda-gcc-specs which means the user can opt-in instead of opt-out?

timsnyder commented 2 years ago

Hey @isuruf. Thanks for the feedback. If you want to override a system header and use #include_next I thought you were supposed to use -isystem or blow away everything with -nostdinc (at least that's what I've read in https://gcc.gnu.org/onlinedocs/gcc/Directory-Options.html). If you were trying to do this for a header using the Centos system gcc that lives in /usr/include or /usr/local/include you'd run into the same problem with using -I.

I was able to play around with the following tests to confirm that -isystem can be used to override the -idirafter and get the effect you require:

set -xe

cat >thing.c <<'END'
#include <a.out.h>
#include <stdio.h>

struct has_a_info{
    unsigned long a_info;
} thing;

// make sure I'm not cheating and we really do include_next the system header
#if !defined (N_MACHTYPE)
#error
#endif

void main() {
    thing.a_info = 0xdeadbeef;
    printf("%#x\n", N_MAGIC(thing));
}
END

$CC -specs=$PWD/specs -o t1 -v thing.c

[[ "$(./t1)" == "0xbeef" ]]

cat >$PREFIX/include/a.out.h <<'END'
#define N_MAGIC(exec) ((exec).a_info << 16 | 0xfeed)

#include_next <a.out.h>
END
$CC -specs=$PWD/specs -I $PREFIX/include -o t2 -v thing.c
# you will see 
# ignoring duplicate directory "/home/centos/.conda/envs/localgcc/include"
# as it is a non-system directory that duplicates a system directory
# 
test "$(./t2)" == "0xbeef" 

$CC -specs=$PWD/specs -isystem $PREFIX/include -o t3 -v thing.c
test "$(./t3)" == "0xdeadbeeffeed"

I had my modified specs in $PWD because the the artifacts in the CI build are almost impossible to identify when the config names get to the length of the ctng-compilers-feedstock configs but they used the sed commands that are checked into install-gcc.sh on this branch.

I also just picked something randomly in the gcc builtin includes that looked like it could be overridden. N_MAGIC was used only because it's #defined inside a #if !defined block.

I like your idea about using %include_noerr so that people can opt into this. I definitely understand how this type of thing might cause a bunch of churn for a use-case that is slightly off the reservation. I'll rework the PR so that we do that. I assume it's cool if I have conda-gcc-specs get created out of this feedstock and have them pin the same version of gcc that they were built with, right?

timsnyder commented 2 years ago

Hey @isuruf, I've finshed reworking my PR to use %include_noerr and I have encountered a problem. It isn't documented anywhere that I've seen but if I'm reading https://github.com/gcc-mirror/gcc/blob/eed9d091de0324453fb34580f550b1ca296d6516/gcc/gcc.cc#L2376-L2379 correctly, you can't use %include, %include_noerr, or %rename in $PREFIX/lib/gcc/$CHOST/${gcc_version}/specs (the 'main' specfile). You can only use them in a specfile passed in via -specs.

How important is the current addition to the specs of only -rpath $PREFIX/lib? Could we not include a default specs file in gcc_impl_ and let people opt into having all of the additions by installing conda-gcc-specs? Or would you want the default behavior to be what it is now and installing conda-gcc-specs would replace the current specs with my expanded ones? This would require some form of mutex package and that seems like a lot to add.

Alternatively, we could patch toplevel gcc to allow %include_noerr to be used in the main specfile. The patch will need to be different across the different versions if for no other reason than the rename of gcc/gcc.c to gcc/gcc.cc.

Thoughts?

timsnyder commented 2 years ago

I retract what I said about the mutex package being too much. After rereading https://conda-forge.org/docs/maintainer/knowledge_base.html?highlight=mutex#openmp it doesn't seem too difficult to put the current specs in a _gcc_specs_mutex that gets installed with gccimpl by default and provide a new _gcc_specs_mutex that swaps out the specs file.

Does it make sense for the cross-compilers to add -rpath $PREFIX/lib to things they build? I guess the assumption is that they will have rpath set to the placeholder and then once they are installed by conda wherever that is, they will work. I can see why you had to make sure that a cmdline passed -rpath was put earlier in the path a few months ago.

timsnyder commented 2 years ago

I see how the _openmp_mutex is swapping out the libgomp.so.1 symlink but what makes conda think that 1_gnu is newer than 1_llvm? Guess I need to go reread the version comparison docs.

beckermr commented 2 years ago

We use track features. Packages with that set are lower priority. See llvm openmp package.

timsnyder commented 2 years ago

I thought that it might have something to do with track_features but when I saw it commented out https://github.com/conda-forge/_openmp_mutex-feedstock/blob/d531c3f3caf9731e879ea1b866b0bf9ba0c7d0a3/recipe/meta.yaml#L19-L23. I stopped thinking that...

beckermr commented 2 years ago

Oh weird! I don't recall that change! I don't know how it works anymore. Sorry!

timsnyder commented 2 years ago

That's okay @beckermr . I appreciate you noticing and replying with what you remember. Maybe since he made the change to remove track_features from the openmp_mutex @isuruf will be able to shed some more light.

In the meantime, I've found https://docs.conda.io/projects/conda/en/latest/user-guide/concepts/packages.html#mutex-metapackages and reread it to understand how track_features works again.

timsnyder commented 2 years ago

I think after reading and thinking about it, the reason Isuru removed track_features from openmp doesn't apply here. Nothing should depend directly on this mutex package with the non-default track_feature pinned. So, I went ahead and finished implementing it that way.

isuruf commented 2 years ago

I'm sorry. I don't remember exactly why that was done and how llvm has lower priority than gnu

isuruf commented 2 years ago

Does it make sense for the cross-compilers to add -rpath $PREFIX/lib to things they build?

Nope

isuruf commented 2 years ago

Alternatively, we could patch toplevel gcc to allow %include_noerr to be used in the main specfile. The patch will need to be different across the different versions if for no other reason than the rename of gcc/gcc.c to gcc/gcc.cc.

Or use sed in the build script to patch it out.

isuruf commented 2 years ago

How important is the current addition to the specs of only -rpath $PREFIX/lib?

Very important. Some build systems ignore LDFLAGS when trying to see if the compiler works and bail out early. (They die with libgcc_s.so or libstdc++.so not found or too old errors)

timsnyder commented 2 years ago

Implementing the mutex package the way I did in https://github.com/conda-forge/ctng-compilers-feedstock/pull/91/commits/cd15f10149a06006bc47dfde58a2f23241db99cd it doesn't work because:

  File "/opt/conda/lib/python3.9/site-packages/conda_build/metadata.py", line 1781, in extract_single_output_text
    output = output_matches[output_index] if output_matches else ''
IndexError: list index out of range

That happens because someone is calling extract_single_output_text with unrendered recipe despite the carefully placed comment saying you shouldn't do that at https://github.com/conda/conda-build/blob/806c3f0c12fd4f0177b396887ba84e1c5f4a1e15/conda_build/metadata.py#L1754-L1759

At this point, I can:

unwind the jinja loop so that the metapackage should work
go back to having only one optional conda-gcc-specs_{cross_target} package and patching gcc toplevel to allow us to use %include_noerr.

Option number 1 seems cleaner to me because you're clearly swapping the specfile in and out with a mutex package. However, I haven't had to maintain those and if you guys think we should patch gcc to allow use of %include_noerr in the toplevel specfile, I'll do it that way.

beckermr commented 2 years ago

Yes. Jinja2 around package outputs is buggy for sure. Best to stick to simple templating and avoid ifs

beckermr commented 2 years ago

Bcc patches sound bad to many folks. Seems cleaner to avoid them per our previous discussions with @isuruf

beckermr commented 2 years ago

*gcc

timsnyder commented 2 years ago

I was thinking the gcc patching wasn't the way to go until @isuruf suggested possibly using sed to patch gcc in this case https://github.com/conda-forge/ctng-compilers-feedstock/pull/91#issuecomment-1081414898 last night.

timsnyder commented 2 years ago

I'll start working on unrolling the loop and see if that works. I'm still going to have two outputs with the same 'name' but different 'build.string' in one recipe and I'm not sure how conda-build will handle that.

I'll also look into submitting a PR with a test for this case but I don't know who is running things over there now that Sarahan is gone.

beckermr commented 2 years ago

You'll get good traction on conda build prs. Jannis and co are really putting energy into the ecosystem.

timsnyder commented 2 years ago

After I unroll the loop on what I have now, and cleaning up the dependencies a bit I have something building that uses the mutex package idea with no patching. The recipe is really ugly because the two variants are unrolled but if it works, I suppose it won't be too bad. If it works in my local build, I'll push it.

timsnyder commented 2 years ago

Well, what I have now starts the build but then doesn't seem to package the gcc-specs-mutex_{cross_target} package before it starts trying to test things and then gcc_impl fails to find gcc-specs-mutex_linux-64 when it tries to test and fails. It's strange. I don't understand why conda-build didn't even try to package those outputs. conda-smithy definitely sees them and added them to the README when i rererendered.

I'm going to go back to what I had where I was just adding a %include_noerr specfile in an optional package and patch gcc.

beckermr commented 2 years ago

Try putting the mutex values as variables in the conda build config and then reference them in the recipe using a single output. Once you rerender this with conda build it may work around the issue.

timsnyder commented 2 years ago

Ok. I can do that. I was loath to since it will double the number of times we build gcc for something that could really be put into the ctng-compiler-activation-feedstock (except that it really does need one file built from gcc build/gcc/specs). I guess I haven't done that because I have been under the impression that we would still want gcc_impl_* to have the same specfile it has had up until now and I wasn't sure about having something from this feedstock depend on the activation scripts. Maybe I'm getting ahead of myself though. I'll try what you suggest and see if it works.

beckermr commented 2 years ago

I think if we carefully apply selectors to the builds it might be ok? I have not tried it. You can and likely should comment out some of the build matrix while testing to speed up rerendering.

isuruf commented 2 years ago

Can we just go to patching gcc? The net effect of using the mutex for the spec file is the same as patching.

timsnyder commented 2 years ago

I'm fine with either implementation. I'm pushing on both of them to see what I can get working. The patch does seem like much less of a headache at this point.

timsnyder commented 2 years ago

The conda-gcc-specs tests are failing because of a stupid awk syntax typo. I'll get that straightened out and also look into improving the selectors.

timsnyder commented 2 years ago

The ppc64le cross s390x build for version 11.2.0 failed with:

Error response from daemon: Head "[https://quay.io/v2/condaforge/linux-anvil-cos7-x86_64/manifests/latest"](https://quay.io/v2/condaforge/linux-anvil-cos7-x86_64/manifests/latest%22): received unexpected HTTP status: 500 Internal Server Error

I don't see controls on Azure to rerun that failing member of the matrix. I can't remember if that's because I need to be a maintainer to see that or you guys don't enable that or Azure doesn't have that feature... I'm going to let the rest keep running. If quay is having some flakiness, I'd rather see if the other 63 make it through.

timsnyder commented 2 years ago

@conda-forge/ctng-compilers this is ready for review. I ended up creating the new conda-gcc-specs_{{cross_target_platform}} package that is optional. The package is only created when cross_target_platform == target_platform because I don't think it makes sense to provide it for cross compilers. However, I kept {{cross_target_platform}} in the name of the package to fit the naming scheme of the other packages in this feedstock.

gcc_impl_{{cross}} specfile now has %include_noerr <conda.specs> at the bottom and still has the -rpath $PREFIX/lib in the same place it was at the end of *link_command:. I wrapped it in !static because I think not having it can create problems if you try to build a static library. I also added -disable_new_dtags because we want DT_RPATH and not DT_RUNPATH. I also added builtin.specs to the gcc_impl package to make it easier for someone to run $CC -specs=builtin.specs and get the same behavior as $CC -specs=<($CC -dumpspecs).

When you install the new optional package, it provides the conda.specs that:

Add -idirafter $PREFIX/include to the end of *cpp_options: and *cc1_options:. Note: cc1plus also uses cc1_options.
Add -rpath $PREFIX/lib -rpath-link $PREFIX/lib -disable_new_dtags -L $PREFIX/lib to the end of *link_command:

The effect of the extra specs is that you can include and link libraries provided by other conda packages without explicitly passing command-line arguments to the gcc driver. I test this behavior in https://github.com/conda-forge/ctng-compilers-feedstock/pull/91/files#diff-f3725a55bf339595bf865fec73bda8ac99f283b0810c205442021f29c06eea9aR208-R211

isuruf commented 2 years ago

However, I kept {{cross_target_platform}} in the name of the package to fit the naming scheme of the other packages in this feedstock.

That's unnecessary given that it's only for native compilers.

https://github.com/orgs/conda-forge/teams/ctng-compilers this is ready for review. I ended up creating the new conda-gcc-specs_{{cross_target_platform}} package that is optional. The package is only created when cross_target_platform == target_platform because I don't think it makes sense to provide it for cross compilers. However, I kept {{cross_target_platform}} in the name of the package to fit the naming scheme of the other packages in this feedstock.

gccimpl{{cross}} specfile now has %include_noerr at the bottom and still has the -rpath $PREFIX/lib in the same place it was at the end of *link_command:. I wrapped it in !static because I think not having it can create problems if you try to build a static library.

Sounds good. static is not for a static library. It is for a statically linked executable, which still doesn't need an rpath.

I also added -disable_new_dtags because we want DT_RPATH and not DT_RUNPATH. I also added builtin.specs to the gcc_impl package to make it easier for someone to run $CC -specs=builtin.specs and get the same behavior as $CC -specs=<($CC -dumpspecs).

Can we not do that? It'll be basically impossible for a user to override this because it's the last argument

Add -idirafter $PREFIX/include to the end of cpp_options: and cc1_options:. Note: cc1plus also uses cc1_options.

Can you explain why -isystem doesn't work in a comment?

Add -rpath $PREFIX/lib -rpath-link $PREFIX/lib -disable_new_dtags -L $PREFIX/lib to the end of *link_command:

Can you move -disable_new_dtags to the front of the options so that a user given --enable_new_dtags can override this?

timsnyder commented 2 years ago

@isuruf and @beckermr, as maintainers, can you rerun https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=485018&view=logs&jobId=029f0d3c-889e-5c3b-8447-1affff42892f for a transient HTTP failure or should I just trigger a complete rerun of CI with an empty commit or close & reopen?

beckermr commented 2 years ago

I think I restarted it.

timsnyder commented 2 years ago

Ok @isuruf, the CI is clean again after removing the spurious deletion you caught yesterday.

beckermr commented 2 years ago

We'll need to add this to the docs. We might also add it is a dep for the compilers meta package.

isuruf commented 2 years ago

Do you have a local build uploaded to anaconda.org that I could try?

timsnyder commented 2 years ago

@isuruf Let me make sure that what I have locally is with the the latest version and I'll upload it for you.

timsnyder commented 2 years ago

@isuruf I ran a local build with what we have ready to merged and uploaded the packages to -c tsnyder. https://anaconda.org/tsnyder/repo

timsnyder commented 2 years ago

We'll need to add this to the docs.

I'll look for a spot and open a PR.

timsnyder commented 2 years ago

So, with regard to the docs, I see three options:

add a sentence or two to https://conda-forge.org/docs/user/faq.html#faq-compiler-metapkg,
create a "Compilers & Runtimes" section in "User Documentation" similar to what we have in https://conda-forge.org/docs/maintainer/infrastructure.html#compilers-and-runtimes but focused on end-users. I would say that everything in the maintainer section applies but focus on what end-users want to know (essentially an expanded version of the FAQ)
Just expand and update https://conda-forge.org/docs/maintainer/infrastructure.html#compilers-and-runtimes

@beckermr & @isuruf, let me know if you guys have thoughts on which would be best.

beckermr commented 2 years ago

We should add to the faq now and then make an issue for an expanded user section to be addressed later IMHO.

timsnyder commented 2 years ago

Added PR for faq at https://github.com/conda-forge/conda-forge.github.io/pull/1685 and issue to add section to the docs https://github.com/conda-forge/conda-forge.github.io/issues/1686

timsnyder commented 2 years ago

@isuruf, I think I've addressed the latest comments. Are we ready to merge?

isuruf commented 2 years ago

Looks good to me. With-idirafter, it cannot be moved to the front with -I, but it can be moved to the middle with -isystem.

conda-forge / ctng-compilers-feedstock

Polish specs for non-cross, tweak config for style #91