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

fortran: update compiler version #1359

Merged
merged 6 commits into from
Oct 12, 2024
Merged

Conversation

xoviat
Copy link
Contributor

@xoviat xoviat commented Mar 20, 2021

No description provided.

@xoviat xoviat requested a review from a team as a code owner March 20, 2021 19:57
@conda-forge-linter
Copy link

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.

@xoviat xoviat changed the title xfortran: update compiler version fortran: update compiler version Mar 20, 2021
@xoviat
Copy link
Contributor Author

xoviat commented Mar 20, 2021

note migration.yml similar to https://github.com/conda-forge/conda-forge-pinning-feedstock/pull/837/files may be required.

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

This change will need to be discussed with @conda-forge/core and probably requires a migration.

@xoviat
Copy link
Contributor Author

xoviat commented Mar 20, 2021

it's been pending for a while now, and wasn't done. I thought I'd shake things loose.

@isuruf
Copy link
Member

isuruf commented Mar 20, 2021

Yes, I don't think there's anything to discuss, but we do need a migration yaml.

@xoviat
Copy link
Contributor Author

xoviat commented Mar 20, 2021

Is this okay?

@beckermr
Copy link
Member

@conda-forge/core Do we want to make an announcement? Should we ship both versions for a while?

@jakirkham
Copy link
Member

Friendly nudge @conda-forge/core (since this came in on the weekend people may have missed it)

@xoviat
Copy link
Contributor Author

xoviat commented Jul 6, 2021

cc @h-vetinari

@isuruf
Copy link
Member

isuruf commented Jul 6, 2021

We still need to add an option to the bot to send this PRs to only packages which use flang on windows. I think right now the bot will send a PR to packages that use fortran on unix, but use m2w64-gcc on windows too.

cc @conda-forge/bot

@h-vetinari
Copy link
Member

Would be awesome if this can be picked up again. 🙃

@zklaus
Copy link
Contributor

zklaus commented Feb 3, 2022

We still need to add an option to the bot to send this PRs to only packages which use flang on windows. I think right now the bot will send a PR to packages that use fortran on unix, but use m2w64-gcc on windows too.

cc @conda-forge/bot

@isuruf, any hint on how to do that? I might give it a shot.

@beckermr
Copy link
Member

beckermr commented Feb 3, 2022

How many windows flang packages are there? Maybe we rebuild a few extra ones if it is small?

@jakirkham
Copy link
Member

How many Fortran packages are there? And how many of those support Windows? Guessing this is probably quite small, but don't really know.

@zklaus
Copy link
Contributor

zklaus commented Feb 4, 2022

@beckermr, @jakirkham, are you saying that we don't need to modify the bot per @isuruf's suggestion, just because even if this would happen (i.e. some packages get superfluous PRs), those are likely so few that we can deal with them just by manually closing the PRs?

Hence, this migrator can be started as is?

@beckermr
Copy link
Member

beckermr commented Feb 4, 2022

That is my suggestion yes. We'll need everyone to agree before we go that way. I also could be wrong.

@isuruf
Copy link
Member

isuruf commented Feb 4, 2022

No. There's 300+ fortran feedstocks and only about 2 dozen feedstocks using flang. I wouldn't characterize that as likely so few.

@beckermr
Copy link
Member

beckermr commented Feb 4, 2022

Ok. So then we’ll need a pr to the bot to avoid rebuilding about 30 feedstocks out of 300. I don’t think I’ll have time to do this but I’m happy to review!

@isuruf
Copy link
Member

isuruf commented Feb 4, 2022

So then we’ll need a pr to the bot to avoid rebuilding about 30 feedstocks out of 300

No, it's the other way around. We have to avoid rebuilding 270 out of 300.

@zklaus
Copy link
Contributor

zklaus commented Feb 4, 2022

Gotcha. Do I understand correctly that this needs a PR to change auto_tick.py?

@isuruf, did you have something general in mind, like an addition to the yaml format for the migrators that allows filtering on the graph, or rather something specific, more like a custom migrator?

@zklaus
Copy link
Contributor

zklaus commented Apr 11, 2022

Sorry, @h-vetinari, I have been taken up by other things. I am not sure when I will manage to pick this up in earnest.

@h-vetinari
Copy link
Member

With llvm-flang hopefully not toooooo far out, I gave this a shot in regro/cf-scripts#1655. Feedback & advice appreciated!

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

@xoviat, not sure if you're still around? 😅

If yes, could you try to add the following (based on regro/cf-scripts#1655)? Since we couldn't write tests I'm a bit apprehensive if it'll work right away, but we can always pause & debug the migration.

PS. Not sure if relevant; clang 11.0 had an ABI break and 11.1 was released to fix it. I had an open PR to rebuild flang for that, but this ran into some weird build tooling errors I wasn't able to debug at the time.

recipe/migrations/flang110.yml Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Member

@isuruf, are your concerns about the migration now addressed? I believe the very old default flang version is the reason why the compilers package is having resolution problems: conda-forge/compilers-feedstock#58

@h-vetinari
Copy link
Member

As an update to anyone subscribed here - we discussed this in a core meeting a few months ago, and the status was that the "updated" flang 11 builds based on classic-flang have no support (we also haven't heard from @xoviat for a long time), and so we decided to wait for llvm-flang to materialise.

We now have flang 17 live in conda-forge, but it's still somewhat experimental, so the earliest to retry this realistically is for flang 18 early next year.

@h-vetinari
Copy link
Member

Another update: flang 18 didn't work for compiling OpenBLAS on windows, but with the in-progress flang 19 (ETA ~September), things are looking much better.

@conda-forge-webservices
Copy link
Contributor

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/meta.yaml) and found it was in an excellent condition.

@h-vetinari h-vetinari marked this pull request as draft August 4, 2024 09:29
@h-vetinari
Copy link
Member

h-vetinari commented Aug 4, 2024

This is still a draft, but it's looking like flang 19 will finally be ready on windows to compile most of our feedstocks. Already tested successfully: OpenBLAS (which still failed with v18), SciPy and a few others in progress.

@h-vetinari h-vetinari marked this pull request as ready for review October 9, 2024 06:56
@h-vetinari h-vetinari dismissed beckermr’s stale review October 9, 2024 06:56

The requested preparation and discussion has happened

@h-vetinari
Copy link
Member

This was discussed several times in core over the last few years (most recently, I had proposed to do this as soon as LLVM 19 goes GA -- there was no opposition to doing this). As we now have all the builds in place, I'm planning to merge this in 48h unless there are other comments.

@h-vetinari
Copy link
Member

Finally the rename from flang-new to flang landed: llvm/llvm-project@06eb10d 🥳

I've tried backporting the patch to LLVM 19, and aside from some trivial conflicts, it looks like we could do it, though I think I'd prefer to leave that for LLVM 20. Let me know if someone thinks differently...

@h-vetinari
Copy link
Member

Alright, let's get this party started!

@h-vetinari h-vetinari merged commit 1858f30 into conda-forge:main Oct 12, 2024
4 checks passed
Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Well I totally missed this, but the migration files need to end in .yaml. We should add a test and/or hook in pre-commit for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants