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

Comments

Clean up win64.mak#3853

Merged
Geod24 merged 1 commit intodlang:masterfrom
kinke:cleanup_win64.mak
Jun 21, 2022
Merged

Clean up win64.mak#3853
Geod24 merged 1 commit intodlang:masterfrom
kinke:cleanup_win64.mak

Conversation

@kinke
Copy link
Contributor

@kinke kinke commented Jun 21, 2022

  • Remove unused variables.
  • Default to a matching MSVC cl.exe in PATH instead of an ancient Visual Studio 2010 installation.
  • Remove explicit 32-bit recipes, requiring a separate make invocation.

@dlang-bot
Copy link
Contributor

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

* Remove unused variables.
* Default to a matching MSVC cl.exe in PATH instead of an ancient
  Visual Studio 2010 installation.
* Remove explicit 32-bit recipes, requiring a separate make
  invocation.
@kinke kinke force-pushed the cleanup_win64.mak branch from 43dc058 to dc20f51 Compare June 21, 2022 18:10
@kinke kinke marked this pull request as ready for review June 21, 2022 18:12
@kinke kinke requested review from andralex and wilzbach as code owners June 21, 2022 18:12
@Geod24 Geod24 merged commit 49a3da0 into dlang:master Jun 21, 2022
@WalterBright
Copy link
Member

@kinke this is good, thanks! A remaining problem is, which make.exe is the makefile designed for?

@kinke
Copy link
Contributor Author

kinke commented Jun 22, 2022

@WalterBright: Azure CI uses DigitalMars make at least: https://github.com/dlang/dmd/blob/75e38a8cda8fc4b0f1bed58299934b91b983b180/.azure-pipelines/windows.sh#L91-L96

I'm not sure whether that's really needed for druntime here though, i.e., whether GNU make wouldn't work. [LDC doesn't use these Makefiles, so I don't know much about them.]

@kinke kinke deleted the cleanup_win64.mak branch June 22, 2022 00:33
kinke added a commit to kinke/phobos that referenced this pull request Jun 22, 2022
* Remove some unused variables.
* Default to a matching MSVC cl.exe & lib.exe in PATH instead of an
  ancient Visual Studio 2010 installation.
* Remove explicit 32-bit recipes, requiring a separate make
  invocation.

Analogous to dlang/druntime#3853.
@rainers
Copy link
Member

rainers commented Jun 22, 2022

The win*.mak files are pretty much tailored for DigitalMars make, as they use its strange and limited include syntax, special command prefixes and lack even simple text processing.

IIRC the main reason was so they can be built with the version of make that is bundled with dmd. Recent releases don't include make anymore, though, so it might be a good opportunity to replace it with a more common make.

kinke added a commit to kinke/phobos that referenced this pull request Jun 24, 2022
* Remove some unused variables.
* Default to a matching MSVC cl.exe & lib.exe in PATH instead of an
  ancient Visual Studio 2010 installation.
* Remove explicit 32-bit recipes, requiring a separate make
  invocation.

Analogous to dlang/druntime#3853.
@WalterBright
Copy link
Member

They don't work with \dm\bin\make.exe anymore.

@kinke
Copy link
Contributor Author

kinke commented Jun 26, 2022

That's CI-tested (see link above), so impossible. I've just tested it (current druntime master) on Windows - make -f win64.mak "HOST_DMD=…\dmd.exe" works a) in an 'x64 native tools VS command prompt' (MSVC env set up) with b) DM make.exe in PATH and c) a matching prebuilt DMD master in ..\dmd\generated\….

Having to specify HOST_DMD explicitly in case there's no dmd.exe in PATH is a hassle; it's apparently only needed for the little copyimports tool. It could at least default to variable DMD (used to compile druntime itself).

@WalterBright
Copy link
Member

what's the date and file size of the make.exe you're using?

@kinke
Copy link
Contributor Author

kinke commented Jun 26, 2022

It's the one from http://downloads.dlang.org/other/dm857c.zip, as used by CI, so from August 19th 2012 (edit: 49,692 bytes).

@kinke
Copy link
Contributor Author

kinke commented Jun 26, 2022

Having to specify HOST_DMD explicitly in case there's no dmd.exe in PATH is a hassle; it's apparently only needed for the little copyimports tool. It could at least default to variable DMD (used to compile druntime itself).

Ah no it can't, as it uses Phobos. Apparently the only reason for its existence and the according HOST_DMD complication are DigitalMars make limitations (#3026)... :D

@WalterBright
Copy link
Member

@kinke thanks!

@WalterBright
Copy link
Member

Well, mea culpa. The update I'd made to make.exe broke it. The old one works.

@WalterBright
Copy link
Member

Seems strange to me to remove from the distribution a critical program needed to build the Windows libraries. It's only 50Kb. It's not like there's a default make program on Windows. Even VC's make program is called nmake.exe.

@WalterBright
Copy link
Member

@ibuclaw
Copy link
Member

ibuclaw commented Jun 27, 2022

Seems strange to me to remove from the distribution a critical program needed to build the Windows libraries. It's only 50Kb. It's not like there's a default make program on Windows. Even VC's make program is called nmake.exe.

Don't you have gmake available?

@mdparker
Copy link
Member

@ibuclaw Even if he does, it's not common at all on Windows. And even for those who do have it, it's often installed as part of MSYS2, in which case it probably isn't on the system path. Since I stopped doing regular C development, I haven't installed any of that stuff for years.

@WalterBright
Copy link
Member

The fewer external dependencies there are, the better. Removing make.exe while still depending on it doesn't make sense.

@adamdruppe
Copy link
Contributor

The distribution should just include gmake.exe. Then it just works and you can use the same thing for windows and linux. Easy solution.

@WalterBright
Copy link
Member

Didn't OSX have problems with different make programs?

RubyTheRoobster pushed a commit to RubyTheRoobster/phobos that referenced this pull request Aug 3, 2022
* Remove some unused variables.
* Default to a matching MSVC cl.exe & lib.exe in PATH instead of an
  ancient Visual Studio 2010 installation.
* Remove explicit 32-bit recipes, requiring a separate make
  invocation.

Analogous to dlang/druntime#3853.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants