[WIP] [NeedsAcceptOrRejectFromWalter] Move standard library config to dmd.conf by marler8997 · Pull Request #9944 · dlang/dmd

marler8997 · 2019-06-02T08:18:05Z

This is a pre-requisite working towards moving the hard-coded standard library configuration out of the compiler (#9936).

This PR moves the logic to determine linker flags for the standard library from the compiler to the tool makedmdconf.d which will use the logic to generate dmd.conf.

This moves the responsibility of configuring the standard library out of the compiler into an external tool.

dlang-bot · 2019-06-02T08:18:07Z

Thanks for your pull request and interest in making D better, @marler8997! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
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 fetch digger
dub run digger -- build "master + dmd#9944"

thewilsonator · 2019-06-02T08:22:32Z

makedmdconf.d

+    flags ~= " -I%@P%/../../../../../phobos";
+    flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/{model}";
+
+    if (os == "windows")


FreeBSD? Also use a switch for this.

thewilsonator · 2019-06-02T08:26:45Z

Is makedmdconf meant to be at top level? Also should we make a /build/ directory for this, src/build.d and whatever else you're going to add in the future? src is already very crowded and unfortunately likely wont change soon.

marler8997 · 2019-06-02T08:36:08Z

It's similar to config.d which is also at the top level, except config.d generates the version file.

wilzbach

Not sure this is a net win as it adds a lot more code to remove a duplication which hardly ever changes.
Also,

Not sure why we always need to rebuild dmd.conf
Windows uses sc.ini for weird historic reasons, so for this PR at least I would focus on making makedmdconf.d work solely for Posix.

wilzbach · 2019-06-03T13:59:57Z

makedmdconf.d

+        }
+    }
+
+    if (os == "linux" || os == "freebsd" || os == "openbsd" || os == "solaris" || os == "dragonflybsd")


Suggested change

if (os == "linux" || os == "freebsd" || os == "openbsd" || os == "solaris" || os == "dragonflybsd")

else if (os == "linux" || os == "freebsd" || os == "openbsd" || os == "solaris" || os == "dragonflybsd")

wilzbach · 2019-06-03T14:00:12Z

makedmdconf.d

+    }
+    if (os == "linux")
+        sharedflags ~= " -L-lpthread -L-lm -L-ldl -L-lrt";
+    if (os == "osx")


Suggested change

if (os == "osx")

else if (os == "osx")

wilzbach · 2019-06-03T14:01:23Z

makedmdconf.d

+    }
+    else
+        mkdirRecurse(path.dirName);
+    std.file.write(path, content);


Could be shorter:

Suggested change

std.file.write(path, content);

content.toFile(path);

wilzbach · 2019-06-03T14:02:44Z

src/build.d

-        target: target,
-        name: "(TX) DMD_CONF",
-        commandFunction: commandFunction,
+    string[] options = new string[0];


Suggested change

string[] options = new string[0];

string[] options;

There's no difference and the appends below will trigger a resize allocation anyhow.

wilzbach · 2019-06-03T14:03:49Z

src/build.d

    void delegate() commandFunction; // a custom dependency command which gets called instead of command
    string name; // name of the dependency that is e.g. written to the CLI when it's executed
    string[] trackSources;
+    bool forceOutOfDate; // always runs the command


maybe name this clearer: forceRebuild

wilzbach · 2019-06-03T14:14:09Z

makedmdconf.d

+    auto helpInfo = getopt(args,
+        "mscoff", &mscoff);
+    enum nonOptionArgCount = 3;
+    const wrongArgCount = (args.length != nonOptionArgCount + 1);


Better make all of these required arguments flags, then the user gets a better error message and you can let getopt do its job.
This will also help when in the future more flags get added (or removed).

wilzbach · 2019-06-03T14:15:08Z

makedmdconf.d

+
+    sharedflags  ~= " -I%@P%/../../../../../druntime/import";
+    sharedflags  ~= " -I%@P%/../../../../../phobos";
+    model32flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/32";


Suggested change

model32flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/32";

string model32flags = " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/32";

wilzbach · 2019-06-03T14:15:20Z

makedmdconf.d

+    sharedflags  ~= " -I%@P%/../../../../../druntime/import";
+    sharedflags  ~= " -I%@P%/../../../../../phobos";
+    model32flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/32";
+    model64flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/64";


Suggested change

model64flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/64";

string model64flags = " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/64";

wilzbach · 2019-06-03T14:16:19Z

makedmdconf.d

+    string model32flags = "";
+    string model64flags = "";
+
+    sharedflags  ~= " -I%@P%/../../../../../druntime/import";


Suggested change

sharedflags ~= " -I%@P%/../../../../../druntime/import";

string sharedflags = " -I%@P%/../../../../../druntime/import";

wilzbach · 2019-06-03T14:19:57Z

makedmdconf.d

+
+    if (os == "windows")
+    {
+        // NOTE: I don't think I need to add user32/kernel32 because I think the


Not a good style to use the first person in remarks as no one knows immediately who you are

Let's better verify before we add such comments. Currently, the sc.ini isn't generated for Windows, but the release sc.ini doesn't look like it needs this: https://github.com/dlang/dmd/blob/master/ini/windows/bin/sc.ini

Yeah the plan isn't to merge this comment, it's to let me and reviewers know it's something that needs to be addressed before merging.

marler8997 · 2019-06-03T14:33:05Z

Right now @WalterBright has questioned whether moving the hard-coded information out of the compiler is justified. I explained the reasoning here #9936 (comment) . If he says no, then this PR and that PR will be axed, so I wouldn't spend any time reviewing these two until then.

thewilsonator · 2019-07-08T07:53:11Z

cc @WalterBright

JinShil · 2019-07-24T08:42:51Z

makedmdconf.d

+import std.path, std.file, std.stdio;
+import std.process;
+
+int main(string[] args)


I'd prefer to have this file integrated into build.d as a new target. The logic for determining whether or not the target needs to be generated and the OS detection logic is already there, and I don't see much of a use case for generating the dmd.conf file outside of a typical build. The makefile can also generate the dmd.conf file by simply calling build.d conf (or whatever the target is called).

marler8997 · 2019-07-24T08:52:12Z

Probably makes sense. It's been too long I don't remember why I originally put it in its own file now.

Still waiting on Walter to respond in #9936 though. He raised concerns on whether moving config to the dmd.conf file is justified. I explained the reasons I am aware of but he hasn't responded yet.

marler8997 · 2020-05-16T18:54:49Z

Cleaning up old PRs that aren't going anywhere.

marler8997 requested review from MartinNowak and wilzbach as code owners June 2, 2019 08:18

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Jun 2, 2019

marler8997 changed the title ~~[WIP] Move more config to dmd.conf~~ [WIP] Move standard library config dmd.conf Jun 2, 2019

marler8997 changed the title ~~[WIP] Move standard library config dmd.conf~~ [WIP] Move standard library config to dmd.conf Jun 2, 2019

thewilsonator reviewed Jun 2, 2019

View reviewed changes

marler8997 force-pushed the genconf branch 11 times, most recently from 010f6d2 to 6a8b776 Compare June 2, 2019 14:57

Move more config to dmd.conf

fe7bdf2

marler8997 force-pushed the genconf branch from 6a8b776 to fe7bdf2 Compare June 2, 2019 15:02

wilzbach reviewed Jun 3, 2019

View reviewed changes

marler8997 changed the title ~~[WIP] Move standard library config to dmd.conf~~ [WIP] [NeedsAcceptOrRejectFromWalter] Move standard library config to dmd.conf Jun 3, 2019

This was referenced Jul 17, 2019

Add -dnostdlib to simplify the use of DLang without stdlib #10180

Closed

@noobject on module decalaration to prevent importing object.d #10188

Closed

JinShil reviewed Jul 24, 2019

View reviewed changes

marler8997 closed this May 16, 2020

	if (os == "linux" \|\| os == "freebsd" \|\| os == "openbsd" \|\| os == "solaris" \|\| os == "dragonflybsd")
	else if (os == "linux" \|\| os == "freebsd" \|\| os == "openbsd" \|\| os == "solaris" \|\| os == "dragonflybsd")

	model32flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/32";
	string model32flags = " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/32";

	model64flags ~= " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/64";
	string model64flags = " -L-L%@P%/../../../../../phobos/generated/{os}/{build}/64";

	sharedflags ~= " -I%@P%/../../../../../druntime/import";
	string sharedflags = " -I%@P%/../../../../../druntime/import";

Uh oh!

Conversation

marler8997 commented Jun 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jun 2, 2019

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewilsonator commented Jun 2, 2019

Uh oh!

marler8997 commented Jun 2, 2019

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Jun 3, 2019

Uh oh!

thewilsonator commented Jul 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 commented Jul 24, 2019

Uh oh!

marler8997 commented May 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

marler8997 commented Jun 2, 2019 •

edited

Loading