Skip to content
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

Add announcement for clang support across platforms #2161

Merged
merged 6 commits into from
May 1, 2024

Conversation

h-vetinari
Copy link
Member

@h-vetinari h-vetinari commented Apr 29, 2024

This fleshes out a skeleton that wrongly got committed in e470f0d - sorry about that.

I had wanted to wait for conda-forge/clang-win-activation-feedstock#34 to land and then raise a PR. @jaimergp kindly pointed out that this slipped in elsewhere.

Relevant xref: conda-forge/ctng-compiler-activation-feedstock#102

@h-vetinari h-vetinari requested review from xhochy and isuruf April 29, 2024 08:45
@h-vetinari h-vetinari requested a review from a team as a code owner April 29, 2024 08:45
Copy link

netlify bot commented Apr 29, 2024

Deploy Preview for conda-forge-previews ready!

Name Link
🔨 Latest commit 2f12df3
🔍 Latest deploy log https://app.netlify.com/sites/conda-forge-previews/deploys/6631675ca9b1470008a51837
😎 Deploy Preview https://deploy-preview-2161--conda-forge-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

```
# please consult @conda-forge/core before doing this
c_compiler:
- clang
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to add something like this:

Suggested change
- clang
# look ma, no selectors!
- clang

@h-vetinari
Copy link
Member Author

pre-commit.ci autofix

pre-commit-ci bot and others added 2 commits April 29, 2024 10:05
@jaimergp
Copy link
Member

pre-commit.ci autofix

Copy link
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me already without the suggested comment.

`recipe/conda_build_config.yaml`:

```yaml
# please consult @conda-forge/core before doing this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was that we don't (yet?) want to make it a free-for-all for feedstocks to choose their compilers. I think the cautious choice would be to keep feedstocks on the default compilers unless there are good reasons for choosing clang, at least while we get some more experience with using clang on linux/win.

That line was meant as a kind of natural "brake" on people's enthusiasm in order to have a softer rollout, but if you feel that this caution is not necessary, I can remove that line.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feeling is that people that choose clang as compiler either know what they are doing and thus don't need to spam core with more notifications or they are doing it as part of a copy/paste from another recipe, fail to apply it and will ping someone competent anyways. Thus, for the sake of a bit less notifications, I would omit it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isuruf, I'm assuming your concern here is addressed now?

Co-authored-by: Mervin Fansler <mmfansler@gmail.com>
@h-vetinari h-vetinari merged commit e76e0b4 into conda-forge:main May 1, 2024
6 checks passed
@h-vetinari h-vetinari deleted the clang branch May 1, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants