use D tool to copy imports instead of explicit list of rules by rainers · Pull Request #3026 · dlang/druntime

rainers · 2020-04-11T06:47:44Z

as an alternative to #3024

This adds a host dmd as an additional dependency, but it's usually available as dmd needs one to build anyway. It might also pave the way for using D to replace other makefile functionality.

In addition, it builds the tool into generated/windows hoping to move other build artifacts there, too.

dlang-bot · 2020-04-11T06:47:46Z

Thanks for your pull request, @rainers!

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#3026"

denizzzka · 2020-04-11T09:42:15Z

std\stdio.d(16): Error: module `stddef` is in file 'core\stdc\stddef.d' which cannot be read
import path[0] = ../druntime\import
Error: cannot find source code for runtime library file 'object.d'
       dmd might not be correctly installed. Run 'dmd -man' for installation instructions.
       config file: not found
import path[0] = ../druntime\import

I faced with this error when forgot to implement creation of ./import/ directory

rainers · 2020-04-11T10:26:35Z

I faced with this error when forgot to implement creation of ./import/ directory

The main problem is to find a host dmd, as the CI scripts are either in a different repo (for azure-pipelines) or virtually immutable (auto-tester).

denizzzka · 2020-04-13T05:01:37Z

auto-tester stalled?

rainers · 2020-04-13T06:06:46Z

auto-tester stalled?

Not sure what happened, the test have passed successfully. I amended a couple of comments to restart the build.

Geod24

Looks okay, but why not go all the way and copy things using a recursive descent instead of relying on mak/WINDOWS ?

Geod24 · 2020-04-13T07:34:51Z

mak/copyimports.d

+        args = args[1..$];
+
+    string importPath = absolutePath("import");
+    string srcPath = absolutePath("src");


This assumes that the script is run from the root project, right ?

Wouldn't the following at the module level be more robust:

immutable RootPath = __FILE_FULL_PATH__.dirName(); immutable ImportPath = RootPath.buildPath("import"); immutable SourceTarget = RootPath.buildPath("src");

?

My initial version allowed the import and src path to be configured via getopt, allowing it to be used in similar situations The current version assumes a bit more about files being passed on the command line, while your proposed change fixes it to the current single use.

It seems a bit strange to pass relative paths on the command line, but then assume other src/import paths internally. I'm not too opinionated, though.

mak/copyimports.d

rainers · 2020-04-13T12:20:56Z

but why not go all the way and copy things using a recursive descent instead of relying on mak/WINDOWS ?

dependency is on mak/COPY and mak/IMPORTS.

There are currently a number of exceptions, namely files in core/sync, that are treated differently (i.e. are processed to di file). No idea why that is the case, though.

Walter has objected the wildcard approach to avoid processing stale or temporary files for dmd, so I didn't try to change it here.

Geod24 · 2020-04-13T13:16:48Z

Walter has objected the wildcard approach to avoid processing stale or temporary files for dmd, so I didn't try to change it here.

Could we then move those files into the tool itself, like it has been done for build.d ?
If you think it is too much change, let me know, I'm fine gong ahead with the current PR.

rainers · 2020-04-13T14:18:06Z

Could we then move those files into the tool itself, like it has been done for build.d ?
If you think it is too much change, let me know, I'm fine gong ahead with the current PR.

I think that belongs into a separate PR as it should happen for all platforms. Otherwise we have new duplications in the build tool and mak/*.

kinke · 2020-04-15T18:32:46Z

Is there anyone actually using these 'elaborately' copied imports on Windows? The DMD 2.091 package contains the full druntime and Phobos src trees, the .7z archive and the installer package too AFAICT, so at least the official Windows packages seem not to need any of this.

Edit: Ah, I hadn't realized the druntime src dir contains an additional import dir with the copies.

Edit2: This will also add 3 currently missing files (core.stdc.errno and core.internal.elf.{dl,io}), good.

kinke · 2020-04-15T18:48:47Z

mak/copyimports.d

+/**
+ * Helper script to copy source files to the `import` directory
+ *
+ * When building druntime, files in `src` are copied to the `import` directory, excluding modules from the `rt` package.


Nit: 3 things are excluded, rt.*, gc.* and test_runner.d. And the .d -> .di conversions in core.sync.* as you've mentioned (LDC doesn't do that).

Slightly adapted the comment, but left out the di-conversion as I plan to copy these files, too.

rainers · 2020-04-16T06:15:40Z

Edit: Ah, I hadn't realized the druntime src dir contains an additional import dir with the copies.

Yeah, the distribution is a bit of a mess. I think it should look more like LDC, not distributing sources of the compiler and the runtime, only "public" imports. For example, adding import test.dub_stdx_allocator; to your code does not raise an error.

rainers · 2020-04-16T06:25:51Z

Does anyone have an idea why the auto-tester keeps reporting "Pass: 8 Pending: 2" while the details show they are all done? Happens on other PRs, too, e.g. #3005. @braddr ?

denizzzka · 2020-04-19T17:48:25Z

buildkite/druntime also failed

rainers · 2020-04-19T19:39:44Z

buildkite/druntime also failed

Looks pretty unrelated, too, as this only changes the Windows build.

- fixup for dlang#3026 - fixes 32-bit builds with ldc (vcvars x86, but x64 default target of ldc)

- fixup for dlang#3026 - fixes 32-bit release builds with ldc (vcvars x86, but x64 default target of ldc)

MartinNowak · 2020-04-28T20:15:32Z

This of course broke the release builder. Weird cause I thought we had at least the Windows part covered by now.

doesn't support tool being build with ldc and x86 target (see forward and use -m$(MODEL) when building tools with host dmd #3075)
does not forward HOST_DC make variable (HOST_DC is still used by dmd, will set HOST_DMD as well MartinNowak/installer@4886449)

Btw, would be nice if the next build system changes didn't land just before the scheduled beta ;).
https://dlang.org/changelog/release-schedule.html

rainers · 2020-04-28T21:30:02Z

This of course broke the release builder.

Sorry for the breakage.

Weird cause I thought we had at least the Windows part covered by now.

The installer CI doesn't automatically run for changes in other repos. Maybe we can make it part of the nightly builds?

doesn't support tool being build with ldc and x86 target (see forward and use -m$(MODEL) when building tools with host dmd #3075)

With the latest LDC, setting LDC_VSDIR_FORCE=1 avoids this trouble as it auto-detects the appropriate environment unconditionally.

does not forward HOST_DC make variable

AFAICT that variable is also deprecated for DMD, see https://github.com/dlang/dmd/blob/05ad63263925b7b2730660f90361b650a3ace8e6/src/posix.mak#L75

- fixup for dlang/druntime#3026

CyberShadow · 2020-11-27T05:48:45Z

This also broke Digger.

CyberShadow · 2020-11-27T05:52:13Z

Another problem with this change is that it reports success despite that this step fails. This causes an error much later, when trying to build Phobos, manifesting as not finding core.stdc.stddef etc.

See dlang/druntime#3026 (comment)

rainers requested review from andralex and wilzbach as code owners April 11, 2020 06:47

rainers mentioned this pull request Apr 11, 2020

~1000 redundant lines removed from mak/WINDOWS #3024

Closed

rainers force-pushed the copy_imports branch 3 times, most recently from aa7e9a7 to 405cdc7 Compare April 11, 2020 08:03

rainers force-pushed the copy_imports branch from 405cdc7 to 69ed765 Compare April 11, 2020 10:19

rainers force-pushed the copy_imports branch from 69ed765 to 8235aaf Compare April 11, 2020 11:19

rainers force-pushed the copy_imports branch from 8235aaf to 63ad8ee Compare April 13, 2020 06:05

rainers force-pushed the copy_imports branch from 63ad8ee to ecdd72f Compare April 13, 2020 06:13

Geod24 reviewed Apr 13, 2020

View reviewed changes

rainers force-pushed the copy_imports branch from ecdd72f to 4dc7aff Compare April 13, 2020 12:33

kinke reviewed Apr 15, 2020

View reviewed changes

use D tool to copy imports instead of explicit list of rules

276f1ea

rainers force-pushed the copy_imports branch from 4dc7aff to 276f1ea Compare April 16, 2020 06:20

thewilsonator approved these changes Apr 20, 2020

View reviewed changes

thewilsonator added the auto-merge label Apr 20, 2020

thewilsonator merged commit dc0c353 into dlang:master Apr 20, 2020

denizzzka mentioned this pull request Apr 21, 2020

Reduces build scripts code duplication #2998

Closed

MartinNowak added a commit to MartinNowak/druntime that referenced this pull request Apr 28, 2020

forward and use -m$(MODEL) when building tools with host dmd

0696624

- fixup for dlang#3026 - fixes 32-bit builds with ldc (vcvars x86, but x64 default target of ldc)

MartinNowak added a commit to MartinNowak/druntime that referenced this pull request Apr 28, 2020

forward and use -m$(MODEL) when building tools with host dmd

08ed96d

- fixup for dlang#3026 - fixes 32-bit release builds with ldc (vcvars x86, but x64 default target of ldc)

MartinNowak mentioned this pull request Apr 28, 2020

forward and use -m$(MODEL) when building tools with host dmd #3075

Merged

MartinNowak added a commit to dlang/installer that referenced this pull request Apr 29, 2020

pass HOST_DC and HOST_DMD to workaround repo differences

4886449

- fixup for dlang/druntime#3026

MartinNowak mentioned this pull request Apr 29, 2020

merge stable dlang/installer#447

Merged

CyberShadow added a commit to CyberShadow/ae that referenced this pull request Nov 27, 2020

ae.sys.d.manager: Fix building Druntime after copyimports.d addition

1c68da9

See dlang/druntime#3026 (comment)

CyberShadow mentioned this pull request Nov 27, 2020

Building 2.092.0 fails CyberShadow/Digger#84

Closed

kinke mentioned this pull request Jun 26, 2022

Clean up win64.mak #3853

Merged

Uh oh!

Comments

Conversation

rainers commented Apr 11, 2020

Uh oh!

dlang-bot commented Apr 11, 2020

Bugzilla references

Testing this PR locally

Uh oh!

denizzzka commented Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Apr 11, 2020

Uh oh!

denizzzka commented Apr 13, 2020

Uh oh!

rainers commented Apr 13, 2020

Uh oh!

Geod24 left a comment

Choose a reason for hiding this comment

Uh oh!

Geod24 Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

rainers Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rainers commented Apr 13, 2020

Uh oh!

Geod24 commented Apr 13, 2020

Uh oh!

rainers commented Apr 13, 2020

Uh oh!

kinke commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinke Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

rainers Apr 16, 2020

Choose a reason for hiding this comment

Uh oh!

rainers commented Apr 16, 2020

Uh oh!

rainers commented Apr 16, 2020

Uh oh!

denizzzka commented Apr 19, 2020

Uh oh!

rainers commented Apr 19, 2020

Uh oh!

MartinNowak commented Apr 28, 2020

Uh oh!

rainers commented Apr 28, 2020

Uh oh!

CyberShadow commented Nov 27, 2020

Uh oh!

CyberShadow commented Nov 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

denizzzka commented Apr 11, 2020 •

edited

Loading

kinke commented Apr 15, 2020 •

edited

Loading