-
-
Notifications
You must be signed in to change notification settings - Fork 305
docs(infrastructure/compilers): Refactor and reword to improve clarity #2596
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
base: main
Are you sure you want to change the base?
Conversation
For now, introduce a few sections as a proposed structure and move the relevant paragraphs unchanged into them. Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
✅ Deploy Preview for conda-forge-previews ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
Some comments for now! The split is definitely an improvement. In further passes I'd like to start splitting this page a bit further.
Also pinging @h-vetinari and @isuruf for additional comments, if there are any.
Thanks!
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
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 working on this
Co-authored-by: h-vetinari <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Okay, I think I've addressed all the remarks, but you'll probably have some ideas how to improve the wording :-). |
As suggested by @isuruf. Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
docs/maintainer/infrastructure.md
Outdated
Alternatively, the specific compiler name can be used as an argument to | ||
the `{{ compiler(...) }}` macro, e.g.: | ||
|
||
```yaml | ||
requirements: | ||
build: | ||
- {{ compiler('clang') }} | ||
- {{ stdlib('c') }} | ||
``` |
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.
TIL! Nice. Is this officially supported or more like a nice byproduct of how compiler()
works (by adding the _{build_platform}
subdir)?
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.
nvm, found my answer in #2596 (comment)
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.
It's supported. If clang_compiler
is set in cbc, it's used. Otherwise clang
is used.
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.
nvm, found my answer in #2596 (comment)
I cannot find the source of that statement in that thread. If you're basing this on my comment about clang
vs. clang-cl
, then I was talking about the values of c_compiler:
in CBC, not the keys used in the compiler()
macro.
TBH, I think {{ compiler('clang') }}
is highly confusing and we should not be supporting it at all in official docs. It works by accident because of the way that {{ compiler("foo") }}
will work if you set foo_compiler:
in CBC and the package-name constructed with <value_of_foo_compiler>_{{ target_platform }}
happens to be legit.
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.
It works by accident because of the way that
Actually, it's even worse than that. If you use {{ compiler("foo") }}
and there's no foo_compiler:
in CBC, conda-build will just construct foo_{{ target_platform }}
, which is completely different from the way the compiler()
macro usually works. Yes, it's long-standing behaviour, but it's confusing as heck, and we should not encourage it IMO.
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 people want a way to select the "official" compilers directly, then that's an OK solution for me. That way we could also do
clang_compiler:
- clang # [unix]
- clang-cl # [win]
to stay MSVC-compatible on windows by default. (and we might want to use clangxx on unix, so that it's similarly C++ capable, without having to resort to {{ compiler("clangxx") }}
)
Could you raise a PR to the pinning repo?
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.
Sorry, is "you" me here? And if yes, then are we talking about just clang
or more compilers?
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.
I had been addressing Isuru, who was in favour of enabling {{ compiler("gcc") }}
and {{ compiler("clang") }}
, and probably has opinions on whether we should default to clang or clang-cl on windows, and whether we should just use the C++ variant for both.
But if you, @mgorny, want to help unblock this, I suggest to open a PR to the pinning repo with the following
# enable `{{ compiler("gcc") }}` / `{{ compiler("clang") }}`; make them C++-capable by default
gcc_compiler:
- gxx
clang_compiler:
- clangxx # [unix]
# stay compatible with MSVC
- clang-cl # [win]
Hopefully @isuruf will have time to provide feedback in that PR directly
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.
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.
I've also applied the suggestions I've proposed above. I'll add compiler('clang')
usage once/if the pinning PR is merged.
I'm very very happy with the results. I think most concerns were addressed, but I'll let this simmer a couple days more in case @isuruf or @h-vetinari have something else to say. Thanks for the awesome work, @mgorny! |
Co-authored-by: jaimergp <[email protected]>
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.
I think there was a misunderstanding about what I meant w.r.t. clang-cl
.
docs/maintainer/infrastructure.md
Outdated
Alternatively, the specific compiler name can be used as an argument to | ||
the `{{ compiler(...) }}` macro, e.g.: | ||
|
||
```yaml | ||
requirements: | ||
build: | ||
- {{ compiler('clang') }} | ||
- {{ stdlib('c') }} | ||
``` |
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.
nvm, found my answer in #2596 (comment)
I cannot find the source of that statement in that thread. If you're basing this on my comment about clang
vs. clang-cl
, then I was talking about the values of c_compiler:
in CBC, not the keys used in the compiler()
macro.
TBH, I think {{ compiler('clang') }}
is highly confusing and we should not be supporting it at all in official docs. It works by accident because of the way that {{ compiler("foo") }}
will work if you set foo_compiler:
in CBC and the package-name constructed with <value_of_foo_compiler>_{{ target_platform }}
happens to be legit.
Add entries to enable explicit `compiler('clang')` and `compiler('gcc')` macro usage. As discussed in conda-forge/conda-forge.github.io#2596, these currently work incidentally, but could be used as a convenient method of forcing GCC or Clang for a particular package. This involves a slight change in behavior — whereas previously the implicit behavior would only activate a C compiler, it now activates both C and C++ compilers. Furthermore, `compiler('clang')` defaults to using `clang-cl` on Windows, making the behavior more consistent with the MSVC usage in the default compiler set. There is currently one feedstock (conda-forge/python-poppler-feedstock) that uses `compiler('clang')` and will need to be tested with the change. A quick search reveals no uses of `compiler('gcc')`. Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
PR Checklist:
docs/
orcommunity/
, you have added it to the sidebar in the corresponding_sidebar.json
file [n/a]This is an attempt to improve structure of the compiler section in the infrastructure page. Currently it consists of a single long block of text with a tiny subsection at the end, with different concepts being mixed together and different audiences being addressed somewhat simultaneously.
What I've done here is:
{{ stdlib('c') }}
macro.This is done from the perspective of someone who's relatively new to conda-forge, but not to the more general topic of packaging and compilers, so it's possible I'm missing something (from both perspectives).