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

Deprecate -m32mscoff #13563

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

thewilsonator
Copy link
Contributor

It is synonymous with -m32 on Windows and cannot be used for other platforms.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! 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 + dmd#13563"

Copy link
Member

@ljmf00 ljmf00 left a comment

Choose a reason for hiding this comment

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

Looks good. Perhaps reference #13110 or https://issues.dlang.org/show_bug.cgi?id=18964 somewhere. The other PR is probably old and there should be some cross-referencing to document it. (either on this or the PR I mentioned, I don't know where it fits the best)

@thewilsonator thewilsonator added the WIP Work In Progress - not ready for review or pulling label Jan 23, 2022
Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

Changelog, both the deprecation and the library renaming.

It is synonymous with `-m32` on Windows and cannot be used for other platforms.
@thewilsonator
Copy link
Contributor Author

@MoonlightSentinel seems like all tests with a // LINK: directive seem to be still looking for phobos32mscoff.lib, I'm not sure where in tools/d_do_test.d this is coming from. Any ideas?

@MoonlightSentinel
Copy link
Contributor

LINK: just changes whether d_do_test passes -c to dmd when compiling the test

@MoonlightSentinel
Copy link
Contributor

The issue here is the switch from MODEL=32mscoff to MODEL=32 - which causes the makefiles to name Druntime / Phobos as druntime32.lib / phobos32.lib instead of druntime32mscoff.lib / phobos32mscoff.lib

@thewilsonator
Copy link
Contributor Author

which causes the makefiles to name Druntime / Phobos as druntime32.lib / phobos32.lib

which is the intended outcome.

An example failing command

\path\to\dmd.exe -conf= -m32 -Ifail_compilation -verrors=0 -Icompilable  -odD:\a\1\s\test\test_results\fail_compilation -ofD:\a\1\s\test\test_results\fail_compilation\needspkgmod_0.exe -i=, fail_compilation\needspkgmod.d

envData.model is clearly 32 here which is what it should be. So the only place I can imagine 32mscoff is coming from is the conf, but we explicitly pass -conf= so I don't understand where it is coming from.

@MoonlightSentinel
Copy link
Contributor

which is the intended outcome.

Okay. run.d currently set the LIB environment variable here:

dmd/test/run.d

Line 535 in 843ae86

env["LIB"] = phobosPath ~ ";" ~ environment.get("LIB");

Note that you'll need to change the MODEL in the druntime / phobos workflows as well.

@MoonlightSentinel
Copy link
Contributor

Relevant configuration files that are currently unchanged:

dmd/src/build.d

Lines 308 to 317 in 4c3701d

enum conf = `[Environment]
DFLAGS="-I%@P%\..\..\..\..\..\druntime\import" "-I%@P%\..\..\..\..\..\phobos"
LIB="%@P%\..\..\..\..\..\phobos"
[Environment64]
DFLAGS=%DFLAGS% -L/OPT:NOICF
[Environment32mscoff]
DFLAGS=%DFLAGS% -L/OPT:NOICF
`;

and

https://github.com/dlang/dmd/blob/master/ini/windows/bin/sc.ini

@dkorpel dkorpel added the Needs Changelog A changelog entry needs to be added to /changelog label Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Changelog A changelog entry needs to be added to /changelog Needs Rebase Needs Work stalled WIP Work In Progress - not ready for review or pulling
Projects
None yet
6 participants