-
-
Notifications
You must be signed in to change notification settings - Fork 608
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
Initial pure D build script #8293
Conversation
|
Thanks for your pull request, @wilzbach! Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#8293" |
src/build.d
Outdated
| } | ||
| Sources sources = { | ||
| frontend: [ | ||
| "arraytypes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the source files need to be explicitly enumerated? I envisioned something more like sourceDir.dirEntries("*.d", SpanMode.depth).join(" ")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to stay as close as possible to the Makefile, but in general there's no big reason not search for them though there are a few dead files which are in the codebase, but never used.
src/build.d
Outdated
|
|
||
| Examples: | ||
|
|
||
| ./run.d dmd # build DMD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be "build.d" instead of "run.d", but I think you can get that dynamically.
efbd9f5
to
e84b802
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so while there's still a ton of things that should be done with this script, it's already quite usable on x86_64 Linux (to verify this I added this script to CircleCi).
Also perfect is the enemy of progress and this PR is already quite big.
So I would suggest we use this as a base for future improvements and PR which should then be a lot easier to review and manage.
Also with an existing build.d other people can more easily join this effort.
Everyone ok with starting of with the minimal subset and slowly increasing it?
| codecov) | ||
| echo "removed - use 'all'" | ||
| # Fall-through is used to maintain compatibility with the existing PRs | ||
| ;& |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I was already touching this code, I removed these long-dead targets.
| rm -rf generated # just to be sure | ||
| # TODO: add support for 32-bit builds | ||
| ./src/build.d MODEL=64 | ||
| ./generated/linux/release/64/dmd --version | grep -v "dirty" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix the 32-bit build in a follow-up PR.
src/build.d
Outdated
| - test on OSX | ||
| - test on Windows | ||
| - allow appending DFLAGS via the environment | ||
| - evaluate whether to use globbing for the file listings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's a long list, but imho it's better to have something as an initial base on which incremental reviews can be done.
Also with an existing build.d other people can more easily join this effort.
src/build.d
Outdated
| auto target = env["G"].buildPath("dmd.conf"); | ||
| auto commandFun = (){ | ||
| conf.toFile(target); | ||
| }; // defined separately to support older D compilers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to a recently DMD bug declaring the function directly doesn't work.
See also:
|
|
||
| case "man": | ||
| "TODO: man".writeln; // TODO | ||
| break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these targets are nice-to-have (as we eventually want to get rid of the Makefile), but obviously not required for building dmd. I added them here, s.t. adding them doesn't get forgotten.
|
|
||
| default: | ||
| case "all": | ||
| goto dmd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to maintain "compatibility" with the Makefile whose default goal was "all" and is just an alias for "dmd".
src/build.d
Outdated
| env.getDefault("C", d.buildPath("backend")); | ||
| env.getDefault("TK", d.buildPath("tk")); | ||
| env.getDefault("ROOT", d.buildPath("root")); | ||
| env.getDefault("EX", d.buildPath("examples")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable isn't used yet, but it will very likely soon be needed (i.e. when the example goal gets added)
| env.getDefault("GIT_HOME", "https://github.com/dlang"); | ||
| env.getDefault("SYSCONFDIR", "/etc"); | ||
| env.getDefault("TMP", "/tmp"); | ||
| env.getDefault("PGO_DIR", srcDir.buildPath("pgo")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable isn't used yet, but once ENABLE_PGO gets added, it should be used.
| { | ||
| env["HOST_DMD_PATH"] = ["which", env["HOST_DMD"]].execute.output.strip; | ||
| env["HOST_DMD_RUN"] = env["HOST_DMD"]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-bootstrapping doesn't make so much sense anymore as it did with the Makefile (after all a D compiler is already required to run this script).
However, the use case of looking the compilation to a specific D version still exists.
src/build.d
Outdated
| { | ||
| // TODO: remove unused file lists | ||
| string[] frontend, lexer, lexerRoot, root, glue, dmd, backend; | ||
| string[] backendHeaders, tkHeaders, backendC, tkC, backendObjects; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use some globbing here to find all the files which would probably cut down half of the size of this script.
The problem is that the frontend is separated in multiple parts and we would have to do a bit of filtering and thus maybe not save so much.
Please do. |
|
Do not work on macOS: Looks like it's missing some flags to compile the C++ code. |
src/build.d
Outdated
| "-Wno-implicit-fallthrough", | ||
| ]; | ||
| else if (env["CXX_KIND"] == "clang++") | ||
| warnings ~= "-Wno-logical-op-parentheses"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check for clang++ should be moved to the else branch of env["ENABLE_WARNINGS"] != "0", i.e. in the else after line 503.
Yes, though I would like to see a brief description of the strategy. |
1f3b4f9
to
f906850
Compare
Yep, sorry but I don't have a macOS machine for testing.
Here's how I plan to proceed over the next weeks:
(2), (4) and (6) might result in multiple PRs. The strategy of this script is to emulate what the Makefile is doing, but without a complicated dependency and dependency system. All individual dependencies of DMD are defined. They have a target path, sources paths and an optional name. When a dependency is needed either its command or custom commandFunction is executed. The function buildDMD defines the build order of its dependencies. |
I haven't tested everything but by fixing the warnings it will now at least build DMD successfully. |
| ./build.d dmd | ||
|
|
||
| TODO: | ||
| - add different ENABLE modes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are "ENABLE modes"? Answered my own question
|
Its appears that we will be bringing up build.d in parallel with the existing makefiles. We are using Circle CI as the CI for build.d. At what point will we replace posix.mak with build.d? After steps 1~3 are completed, can we make the switch? |
|
As much as I really want this to happen, I think we should wait to pull this until @WalterBright is finished porting the backend to D. That process seems to involve a lot of makefile changes, and putting the burden on @WalterBright and reviewers to modify and verify two different builds scripts is going to be a bit of a nuisance. |
In theory we could already make the switch for e.g. CircleCi (only the enable modes are missing and they are quick to add), though I wanted to wait a few months to make sure there aren't any sneaky bugs in the build script somewhere and as you mentioned yourself it makes sense to wait with adding this build script until the backend conversion is complete.
Fair enough, but how about adding the script without adding it to the CIs (for now)? Though of course, we could replace the hard-coded file list with file globbing and thus eliminate the need for manually updating this build script during the backend conversion. |
But we need to test it as we go.
That would be great. If you can pull it off, I'd support adding it to the CI now, but we should be at @WalterBright's beck and call should he run into any issues. |
e2617f0
to
3060159
Compare
I think I managed to do so. |
src/build.d
Outdated
| return "dragonflybsd"; | ||
| else version(Solaris) | ||
| return "solaris"; | ||
| else version(SunOS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a predefined version identifier?
src/build.d
Outdated
| */ | ||
| auto detectModel() | ||
| { | ||
| version(Solaris) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should SunOS be used here as well? Or should detectOS be used instead?
f6dec02
to
e417f0e
Compare
|
BTW when the recent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a big step in the right direction. Let's move forward. I don't want this to hang around in an unfinished state for years, though. I hope we can keep at it regularly until it's done.
|
|
||
| void main(string[] args) | ||
| { | ||
| int jobs = totalCPUs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is never used. A primary feature of a build system is to facilitate parallel builds. Are there plans to implement such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It always was (though of course the user should have been able to regulate this with -j):
https://github.com/wilzbach/dmd/blob/e417f0e0dfd3ea117609cd48a9de53695aaaa3ad/src/build.d#L244
This is still the case for master:
Lines 141 to 142 in 9e877e0
| foreach (target; targets.parallel(1)) | |
| target(); |
Lines 1317 to 1320 in 9e877e0
| foreach (dep; deps.parallel(1)) | |
| { | |
| dep.run(); | |
| } |
(inspired by @JinShil's recent dream on the NG about being able to build DMD easily on any platform)
Similarly to the
test/run.d, this allows building DMD from source without depending on the Makefile.At the moment it's a simplistic translation of more or the less the bare minimum of the
posix.makwhich is needed to build DMD, but the following is intended (not in this PR though):I opened this PR to avoid duplicated efforts and give others already a sneak-peek at this.
This already compiles DMD on Linux and even supports partial and parallel compilation (almost as good the Make build).
I kept the rule system as simplistic as possible to avoid adding a huge chunk of code that we would have to maintain + it's not really needed here anyhow.
Also the builder script got quite long, because instead of using one text blob for the source files, I used single lines for each file to avoid the merge conflicts that sometimes happen (though I have no strong feeling on this style).
Why such a naive approach and not cmake, ninja, reggae, ...?
There has never been a consensus on which third-party build system should be used.
Using a pure D script avoids any further dependencies and after all, D should be powerful to handle any task!
Also the dependency flow is rather simplistic here. The backend and lexer need to be built before the main DMD built, but they can be built in parallel once the initial dependencies have been generated.
But the Makefile is working so nicely
Well, I guess I'm one of the very few persons who understands the
posix.mak(it looks more scary than it is), but I doubt many others do.Also we there have been numerous complaints about the more or less unmaintained win32.mak and win64.mak files.
For now, like
run.dthis is an experiment and only intended to slowly replace the Makefiles once it has been thoroughly tested.But isn't depending on D another dependency?
Nope, D requires a host D compiler since 2015. The Makefile comes with an AUTO_BOOTSTRAP option to automatically download a D compiler. While I added support for this in
build.dtoo (it makes sense when you want to lock the D compiler version to a specific one), it's only of limited help to someone on a fresh machine without a D compiler.Imho we can expect a D compiler dev to have a working D installation, but if this AUTO_BOOTSTRAP option is convenient for some (I think the Jenkins CI uses it), we could opt to add a small Bash bootstrapping script around it (or keep this part of the Makefile).
What's missing from this PR?
This PR contains everything essential to use the
build.dscript on x86_64 Linux.Though there's still much work that needs to be done in future PRs.
Check the TODO header.
What won't be part of this PR?
I don't plan to support Windows nor all targets of posix.mak in this initial PR.