-
Notifications
You must be signed in to change notification settings - Fork 45
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
Remove fortran binary files and add logic to build them from source #152
Conversation
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.
Nice! I think this is a good step forward. I made a bunch of minor comments; the only one that really stuck out to me is a question of whether the newly-built Fortran binaries need to be included in released artifacts.
# 1. Force to only build the 64-bit version since ITT API 32-bit support will be discontinued soon | ||
# 2. Disable PT support for MacOS since we have x86 specific assembly instruction | ||
# 3. Switch to use Ninja CMake Generator for Windows since setup-fortran action | ||
# doesn't work in case of CMake + VS (https://github.com/fortran-lang/setup-fortran/issues/45) |
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.
Good documentation! I can foresee people stumbling on this at some point.
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 hope this isn't trolling 😀
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.
No, no! I was just reading through this PR thinking, "oof, I hope I don't have to mess with this Fortran stuff in CI because I won't remember what works and what doesn't"! These doc comments came along at the right time.
.github/workflows/release.yml
Outdated
- name: Build C library | ||
run: python buildall.py ${{ runner.os != 'macOS' && '-pt -v' || '-v' }} | ||
run: python buildall.py -v ${{ runner.os != 'macOS' && '-pt' || '' }} |
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.
Don't we want the release artifacts to contain the Fortran binaries? I would have expected an -ft
here or something like that. To be sure, I have never used those binaries so I don't mind one way or another, but I do think the previous behavior was that the Fortran binaries were somehow included, no?
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 agree with you. So I added a build of everything we provide into the release package and included fortran binaries. I think in the future it would be fine to add the Rust and Python packages to the release as well.
Details:
-ft
build option--cmake_gen
to specify the CMake build generator on Windows (fixed Ninja flow)