Skip to content

Improve windows build #428

Merged
ewu63 merged 10 commits intomdolab:mainfrom
whophil:feature/win-build
Apr 1, 2025
Merged

Improve windows build #428
ewu63 merged 10 commits intomdolab:mainfrom
whophil:feature/win-build

Conversation

@whophil
Copy link
Contributor

@whophil whophil commented Mar 29, 2025

Purpose

This PR improves the apparently flaky Windows build by making it better match the conda-forge build.

Flang is pinned to 5, which is the same as the current conda-forge global pin. The previous builds were using "latest" Flang, which resolved to 19. I'm not sure why the Flang 19 builds were flaky.

Note that conda-forge is currently migrating to Flang 19. As more conda-forge projects migrate to Flang 19, we can consider updating the CI here to Flang 19, leaning on the experience of the increasing number of conda-forge packages using Flang 19.

Expected time until merged

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (non-backwards-compatible fix or feature)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Documentation update
  • Maintenance update
  • Other (please describe)

Testing

Checklist

  • I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@whophil whophil requested a review from a team as a code owner March 29, 2025 18:16
@whophil whophil requested review from lamkina and marcomangano March 29, 2025 18:16
This was referenced Mar 29, 2025
@codecov
Copy link

codecov bot commented Mar 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.30%. Comparing base (cdd8d96) to head (0a1d081).
Report is 11 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #428       +/-   ##
===========================================
- Coverage   74.98%   63.30%   -11.69%     
===========================================
  Files          22       22               
  Lines        3338     3338               
===========================================
- Hits         2503     2113      -390     
- Misses        835     1225      +390     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@whophil
Copy link
Contributor Author

whophil commented Mar 29, 2025

A few comments:

  1. Not really sure why the coverage is down
  2. I am fine with any tweaks the maintainers may want to make to setup.py as long as the meson configure/build logs can, perhaps optionally, be redirected to stdout upon success.

@ewu63
Copy link
Collaborator

ewu63 commented Mar 31, 2025

Changes look good, just left a few minor comments. I do think in the medium term we should probably move to mesonpy as that project seems mature, at the time we decided to wait because it seems it would get merged back to meson but that no longer appears to be the plan.

@whophil
Copy link
Contributor Author

whophil commented Mar 31, 2025

I do think in the medium term we should probably move to mesonpy as that project seems mature

Agree, I think with meson-py a large amout of boilerplate can be outsourced.

Copy link
Collaborator

@marcomangano marcomangano left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @whophil ! I am onboard with the more verbose printouts. Not familiar with meson-py @ewu63 but I am interested in anything that can make the build process easier to maintain and tweak.

if p1.returncode != 0:
with open(setup_log, "r") as f:
print(f.read())
raise OSError(sysargs, f"The meson setup command failed! Check the log at {setup_log} for more information.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could improve the error message slightly by mentioning that it is also printed above. Bonus points with adding a header/footer to print(p1.stdout) to clearly delineate which part is from the log. Can do the same with p2 below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair points. Given the imminent switch to meson-py (I can work on it), I would also be OK if one of the maintainers wanted to merge as-is.

@ewu63
Copy link
Collaborator

ewu63 commented Apr 1, 2025

Alright, merging and then hopefully the other PRs can go through too.

@ewu63 ewu63 merged commit 85757f5 into mdolab:main Apr 1, 2025
13 of 14 checks passed
@ewu63
Copy link
Collaborator

ewu63 commented Apr 1, 2025

FYI @whophil the coverage went down because this PR was initiated on your branch, and therefore CI skips testing some proprietary optimizers (such as SNOPT).

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.

3 participants