Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Make win32.mak compatible with MODEL=32omf#3703

Merged
dlang-bot merged 1 commit intodlang:masterfrom
MoonlightSentinel:ci/fix-windows-default-switch
Jan 25, 2022
Merged

Make win32.mak compatible with MODEL=32omf#3703
dlang-bot merged 1 commit intodlang:masterfrom
MoonlightSentinel:ci/fix-windows-default-switch

Conversation

@MoonlightSentinel
Copy link
Contributor

@MoonlightSentinel MoonlightSentinel commented Jan 24, 2022

The previous assumption was MODEL=32 because MODEL=32mscoff is built by win64.mak.

Verified locally because Azure requires further changes outside of this repository.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3703"

@MoonlightSentinel MoonlightSentinel force-pushed the ci/fix-windows-default-switch branch 3 times, most recently from ca82a78 to 61cdd7a Compare January 24, 2022 16:02
@MoonlightSentinel MoonlightSentinel changed the title Azure: Fix OMF build Make win32.mak compatible with MODEL=32omf Jan 24, 2022
@MoonlightSentinel MoonlightSentinel marked this pull request as ready for review January 24, 2022 16:21
The previous assumption was `MODEL=32` because `MODEL=32mscoff` is
built by `win64.mak`.

Verified locally because Azure requires further changes outside of this
repository.
@MoonlightSentinel MoonlightSentinel force-pushed the ci/fix-windows-default-switch branch from 61cdd7a to dafc74a Compare January 24, 2022 17:00
@MoonlightSentinel
Copy link
Contributor Author

This will need to be force-merged because it requires additional changes to dmd/windows.sh and phobos/win32.mak

@MoonlightSentinel MoonlightSentinel marked this pull request as draft January 24, 2022 17:16
@RazvanN7
Copy link
Contributor

@MoonlightSentinel Ping me when you need this merged.

@MoonlightSentinel
Copy link
Contributor Author

Submitted the required changes to windows.sh here: dlang/dmd#13572. That should enable Azure to build Druntime with MODEL=32omf again (but still fail on Phobos - will open another PR for that).

@MoonlightSentinel MoonlightSentinel marked this pull request as ready for review January 25, 2022 14:25
@MoonlightSentinel
Copy link
Contributor Author

Azure successfully built druntime! Just one patch left... (Phobos/win32.mak)

@MoonlightSentinel
Copy link
Contributor Author

Can you temporarily mark the Azure job as not required? Or do we have to modify the configuration until the Phobos PR is merged?

@RazvanN7
Copy link
Contributor

I don't know how to do that and I might not have the rights, however, I can force merge this if that is required.

@MoonlightSentinel
Copy link
Contributor Author

It'll be required - neither makefile update is sufficient on its own (this fails due to the outdated phobos/win32.mak and a Phobos PR cannot build Druntime without this patch)

@RazvanN7
Copy link
Contributor

@MoonlightSentinel Thanks for fixing this!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants