tools icon indicating copy to clipboard operation
tools copied to clipboard

Fix LDC arm64 build

Open lionello opened this issue 2 years ago • 6 comments

This improves upon https://github.com/dlang/dmd/pull/13288 and adds a copy of osmodel.mak into this repo, to break the build dependency between the two repos. This allows one to build the tools with ldc, without the need to clone dmd.

This PR also adds a new make var ARCH which is the llvm/dub compatible architecture (ie. x86_64, x86, or aarch64). This replaces the previous subst hack on line 42, which could never recover arm64 from just the MODEL 64.

Related bugzilla, raising a similar issue: https://issues.dlang.org/show_bug.cgi?id=22078

lionello avatar Nov 24 '21 04:11 lionello

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

dlang-bot avatar Nov 24 '21 04:11 dlang-bot

@wilzbach @CyberShadow could you guys have a look at this when you have time?

lionello avatar Nov 29 '21 00:11 lionello

I thought reusing the include file was a feature. Perhaps it would make sense to use the one from Druntime instead, if the DMD repository is not otherwise useful in these circumstances?

CyberShadow avatar Nov 29 '21 06:11 CyberShadow

I thought reusing the include file was a feature. Perhaps it would make sense to use the one from Druntime instead, if the DMD repository is not otherwise useful in these circumstances?

I think the point here is to break the dependency between the two repositories. The current way could not easily do reproducible builds and it is inefficient to clone a whole repo for a single file. Although I think the Makefile is mostly used internally anyway, so we could just have fallbacks to dmd/druntime/phobos and hang if none of them has it, but I think this is the most reasonable way if we want to stick with old GNU Makefiles.

ljmf00 avatar Nov 29 '21 14:11 ljmf00

I think the point here is to break the dependency between the two repositories. The current way could not easily do reproducible builds and it is inefficient to clone a whole repo for a single file.

Then maybe download the single file instead of cloning the entire repo?

curl -L -O https://raw.githubusercontent.com/dlang/dmd/master/src/osmodel.mak

The file can even be pinned to a certain reveision:

curl -L -O  https://raw.githubusercontent.com/dlang/dmd/f87bc623ea3b3a722c50ecb0a6c318add1a35916/src/osmodel.mak

Copying the file here will just "hide" the dependency because future changes will have to be applied to each copy.

MoonlightSentinel avatar Nov 29 '21 14:11 MoonlightSentinel

I have submitted a PR for the osmodel.mak changes to dmd, but I still think it's excessive to have a cross-repo dependency for such a trivial file.

Once https://github.com/dlang/dmd/pull/13428 is merged I can update this PR to use curl instead of git clone.

lionello avatar Dec 14 '21 15:12 lionello

The repos changed quite a lot. Closing.

lionello avatar Sep 02 '22 17:09 lionello