Generate all .o .deps and binaries to generated/ #6562

RazvanN7 · 2017-02-23T15:21:15Z

I created a directory src/ddmd where I moved all the files in src/, except makefiles. Other than that I modified optabgen.d to generate sources to src/ddmd/backend and I also moved tk.c to backend since it was the only .c file in src/ddmd.

I also modified the makefile to have all generated files (except .c .d and .h files - source code generated files) in a directory called generated/ which is on the root level (same level as src). After creating dmd, the executable is copied to src/dmd so that other tools aren't affected by this PR.

Now all the other tools can be updated to use the dmd from the generated/ folder whithout causing a major stall. After all the scripts are updated to use generated/dmd/ I will issue a new PR which renames ddmd to dmd and no longer generates dmd in the src folder. This way things will get smoothly in the right direction.

This is a WIP as I only applied the modifications to the posix makefile.

This is a duplicate of #6489 since it was more time consuming to apply these changes on that branch.

wilzbach · 2017-02-23T15:48:52Z

src/posix.mak

+
+GENERATED = ../generated
+G = $(GENERATED)/$(OS)$(MODEL)
+$(shell mkdir -p $G)


How about following the style used at Phobos and Druntime?

I think that there is no good reason to have two unused levels in the hierarchy

I think that there is no good reason to have two unused levels in the hierarchy

Well you can do debug and release builds at the same time without any issues ... and there are tons of other different build types available for DMD: https://github.com/dlang/dmd/blob/master/src/posix.mak#L160

This is interesting. A few explanations follow.

Currently the code does not make the situation any worse than before - it just moves all generated files elsewhere, indiscriminately.

The structure generated/$(OS)$(MODEL) (as opposed to Phobos' generated/$(OS)/$(MODEL)) was chosen by @RazvanN7 and I after discussing for a while and figuring there's no real need for two separate levels in the dir hierarchy.

We could actually simply use generated/ without the OS or the model, BUT:

It is reasonable to generate 32 and 64 binaries on a given machine

Even on a given OS, there may be multiple "OS"s - consider generating Windows and Cygwin binaries on Windows. Are there any others? Not sure how important this is, but putting the OS in the dirname helps such situations.

Now (this is getting to @wilzbach's point), we have the situation in which changing CFLAGS and DFLAGS affects the generated binaries in many ways, per https://github.com/dlang/dmd/blob/master/src/posix.mak#L160. The "total" solution to that is to canonicalize and place the controlling flags in the dir names, i.e. (after removing ENABLE) in alphabetical order: COVERAGE, DEBUG, LTO, PGO_GENERATE, PGO_USE, PROFILE, PROFILING, RELEASE, UNITTEST. Then the dir name of a generated bunch of binaries would be something like generated/posix64/DEBUG_PROFLE_UNITTEST. The "final release" version should sit in generated/$(OS)$(MODEL/ so flags do not affect tool developers. This scheme may be overkill even if technically appropriate. I think @WalterBright and @MartinNowak should chime in here because they'll be the first and most affected.

This is interesting. A few explanations follow.

The structure generated/$(OS)$(MODEL) (as opposed to Phobos' generated/$(OS)/$(MODEL)) was chosen by @RazvanN7 and I after discussing for a while and figuring there's no real need for two separate levels in the dir hierarchy.

I doesn't cost anything either, and consistency across repos seems more important (in particular for scripting, e.g. release scripts). So let's please just stick with generated/$OS/$BUILD/$MODEL as anywhere else.

Even on a given OS, there may be multiple "OS"s - consider generating Windows and Cygwin binaries on Windows. Are there any others? Not sure how important this is, but putting the OS in the dirname helps such situations.

I also allows to more easily rsync code from other machines/VMs.

Now (this is getting to @wilzbach's point), we have the situation in which changing CFLAGS and DFLAGS affects the generated binaries in many ways, per https://github.com/dlang/dmd/blob/master/src/posix.mak#L160. The "total" solution to that is to canonicalize and place the controlling flags in the dir names, i.e. (after removing ENABLE) in alphabetical order: COVERAGE, DEBUG, LTO, PGO_GENERATE, PGO_USE, PROFILE, PROFILING, RELEASE, UNITTEST.

Those are hardly maintained and rarely used, let's not overshoot and leave it to maintainers.
Separating debug and relase builds is quite helpful though.
An alternative would be to dump all flags to some file and error on mismatch (requiring a clean step). This is the usual configure/Make approach to changing flags.

OK with @MartinNowak, @RazvanN7 can you please execute? Thanks!

So, what will be the final path of the created dmd binary when make is invoked with the default settings?

In the end, the path will be generated/dmd, but until then we will keep a copy of dmd in src/ so that we will have a smooth transition

the path will be generated/dmd

Didn't we agree on generated/linux/debug/64/dmd ?
In the final build step you could create a symlink to the last built binary (e.g generated/dmd -> $(abspath generated/linux/debug/64/dmd)`) if you really want to ...

That's right. Sorry, I misread the comments

RazvanN7 · 2017-02-23T16:00:04Z

I don't have a windows setup, so my strategy is to spam the auto-tester. If someone with a windows machine wishes to co-op this PR, be my guest.

UplinkCoder · 2017-02-23T16:42:39Z

@RazvanN7 please give me the rational for using the generated folder.
IMO it's easier to work with dmd when the .o files are in the same dir as the sources.

wilzbach · 2017-02-23T16:46:14Z

please give me the rational for using the generated folder.

#6489

andralex · 2017-02-24T16:05:10Z

One more thing: @RazvanN7 mentioned the file src/checkwhitespace.d is actually a tool that has no place in the dmd project (it uses phobos, it's also used by phobos and dmd). For this diff @RazvanN7 please leave checkwhitespace.d in src/ to avoid breakage, and let's move it in a later step. Thanks!

skl131313 · 2017-02-24T19:37:50Z

With all the changes happening anyways, would there be a possibility of adding druntime/phobos as submodules into dmd? The make files expect a specific file structure for where dmd, druntime, and phobos should be located relative to each other. Having them be submodules in dmd will make building from source easier. It will also know which specific version of druntime/phobos to use, rather than having to checkout the correct version for every repo.

MartinNowak · 2017-02-25T14:02:14Z

IMO it's easier to work with dmd when the .o files are in the same dir as the sources.

Building out of tree is a common best practice and immediately useful to separate debug and release objects to avoid errors during development.

With all the changes happening anyways, would there be a possibility of adding druntime/phobos as submodules into dmd?

Let's please not open another can of worms. @CyberShadow has an exterior repo that tracks all repos, https://bitbucket.org/cybershadow/d (used by Digger).

MartinNowak · 2017-02-25T14:19:28Z

Let's not move src/checkwhitespace.d → src/ddmd/checkwhitespace.d, right? Will also fix your tests ;).

skl131313 · 2017-02-25T21:05:24Z

@MartinNowak

This is probably the only time to do so. A lot of the changes that would have to be made are on the same lines that this change is going to cause. Relying on an external tool and external repo for building kind of proves my point.

RazvanN7 · 2017-02-27T10:28:49Z

The last commit was called "Added $(BUILD) to generated path", but it seems that $(BUILD) was evaluated to the empty string.

CyberShadow · 2017-02-27T10:31:16Z

The last commit was called "Added $(BUILD) to generated path", but it seems that $(BUILD) was evaluated to the empty string.

I believe the problem occurred at the time the commit is created - your shell evaluated $(BUILD) to the expansion of the output of a non-existent command. To avoid this, you can use single quotes when specifying the commit message on the command line, or don't use -m and type the commit message in the invoked editor instead.

RazvanN7 · 2017-02-27T10:33:06Z

@CyberShadow thanks! I should have known that this would happen.

andralex · 2017-02-27T14:15:01Z

Looking green! Well. almost. What's the failure with DAutoTest? Thanks! cc @CyberShadow maybe you could help.

CyberShadow · 2017-02-27T14:31:13Z

make[2]: *** No rule to make target 'project.ddoc', needed by '/dev/shm/dtest/work/repo/dlang.org/web/dmd-prerelease/.generated'.  Stop.

Is it the DMD source code DDoc stuff? Not really familiar with that.

CyberShadow · 2017-02-27T15:15:03Z

However, I think @RazvanN7 has deliberately made two commits to separate the renaming from the Makefile adjustments, so imho there's no need to do squashing here...

No, the first commit moves the file and the second one moves it back. So a squash is fine in this case.

andralex · 2017-02-27T15:16:40Z

way we go...

CyberShadow · 2017-02-27T15:18:02Z

So, what will be the final path of the created dmd binary when make is invoked with the default settings?

Question still unanswered in light of the latest changes :) Need to know what to update DIgger to use, so that we can delete dmd/src.

wilzbach · 2017-02-27T15:19:49Z

Question still unanswered in light of the latest changes :) Need to know what to update DIgger to use, so that we can delete dmd/src..

AFAIK it's supposed to be generated/linux/release/64/dmd (?)

RazvanN7 · 2017-02-27T15:41:22Z

@CyberShadow : @wilzbach is right. that will be the path, same as in the phobos makefile

CyberShadow · 2017-02-27T15:49:42Z

Hmm, dlang.org autodeploy build failed:

digger: Running: 'make' '-f' 'posix.mak' 'MODEL=64' 'HOST_DC='
digger: PATH=/bin:/usr/bin
no cpu specified, assuming X86
posix.mak:81: *** 'dmd' not found, get a D compiler or make AUTO_BOOTSTRAP=1.  Stop.

CyberShadow · 2017-02-27T15:52:26Z

Something changed in this PR which for some reason kept working on the documentation tester but didn't work on the dlang.org autodeployer, even though they use nearly the same environment and build commands.

CyberShadow · 2017-02-27T15:54:33Z

Found the bug: if (buildPath(sourceDir, "src", "idgen.d").exists)

Odd that it worked on DAutoTest :)

CyberShadow · 2017-02-27T16:00:11Z

Fixed. CyberShadow/ae@13bc434

MartinNowak · 2017-02-27T23:31:49Z

.gitignore still needs to be updated

MartinNowak · 2017-03-02T12:59:49Z

Seems like this broken the build script @RazvanN7, could you please have a look.
http://nightlies.dlang.org/dmd-master-2017-03-01/build.html

Running: make MODEL=32 DMD=..\dmd\src\dmd HOST_DC=C:\Users\vagrant\old-dmd\dmd2\windows\bin\dmd.exe RELEASE=1 LATEST=master -f win32.mak dmd
if not exist "..\generated\Windows_NT32" mkdir ..\generated\Windows_NT32
make -fwin32.mak C=ddmd\backend TK=ddmd\tk ROOT=ddmd\root MAKE="make" HOST_DC="C:\Users\vagrant\old-dmd\dmd2\windows\bin\dmd.exe" DMODEL= CC="dmc" LIB="lib" OBJ_MSVC="" "OPT=-o" "DEBUG=" "DDEBUG=" "DOPT=-O -release -inline" "LFLAGS=-L/delexe/la" ..\generated\Windows_NT32\dmd.exe
Error: don't know how to make '..\generated\Windows_NT32\dmd.exe'

CyberShadow · 2017-03-02T13:01:13Z

@MartinNowak Since so many things seem to have broken nightly builds recently, is there a possibility of adding it as a CI? (Bonus: you can download a version of D that includes a certain PR to test it out)

RazvanN7 · 2017-03-02T13:14:49Z

@MartinNowak it seems that the rule to make the target ..\generated\Windows...\dmd.exe doesn't exist in the makefile. That is odd, since the rule definitely exists in the makefile that I commited. The link you provided doesn't output this error. Could you, please, give me some more information?

MartinNowak · 2017-03-02T13:27:49Z

The cost for a real run is prohibitive atm. (too many OSes/VMs).

MartinNowak · 2017-03-02T13:28:20Z

http://nightlies.dlang.org/dmd-master-2017-03-02/build.html, wrong date, my mistake.

CyberShadow · 2017-03-02T13:34:59Z

The cost for a real run is prohibitive atm. (too many OSes/VMs).

How about making it on-demand, or just whenever a makefile changes?

MartinNowak · 2017-03-02T13:40:41Z

Yes, we should work on replacing the create_dmd_release script and CI integration (heuristic sounds like a good idea) in the longer run, but I'm busy w/ other things atm.
https://trello.com/c/uDoxVgpc/110-ci-for-nightly-builds

MartinNowak · 2017-03-03T18:07:37Z

See #6585.

wilzbach · 2017-03-04T05:15:08Z

Eww discovered a couple of more issues with the "new" Makefile:

#6586

RazvanN7 force-pushed the ddmd_generated branch 2 times, most recently from 3b9ece9 to 72278c1 Compare February 23, 2017 15:45

wilzbach reviewed Feb 23, 2017

View reviewed changes

RazvanN7 force-pushed the ddmd_generated branch from 72278c1 to 0039af1 Compare February 23, 2017 16:35

RazvanN7 force-pushed the ddmd_generated branch 3 times, most recently from 9fac9bb to 87e0612 Compare February 24, 2017 07:59

RazvanN7 mentioned this pull request Feb 24, 2017

Delete all ddmd. from modules #6489

Closed

RazvanN7 force-pushed the ddmd_generated branch 7 times, most recently from df17adb to 060531b Compare February 27, 2017 09:30

Added to generated path

e27753a

RazvanN7 force-pushed the ddmd_generated branch from 060531b to e27753a Compare February 27, 2017 10:27

Uncommented previously commented if at line 522

a745a9e

RazvanN7 force-pushed the ddmd_generated branch from 9371658 to a745a9e Compare February 27, 2017 15:15

andralex added the Merge:auto-merge label Feb 27, 2017

dlang-bot merged commit 2bf9a9d into dlang:master Feb 27, 2017

RazvanN7 deleted the ddmd_generated branch February 28, 2017 10:08

RazvanN7 mentioned this pull request Feb 28, 2017

Moved everything that is generated to the generated folder #6580

Merged

RazvanN7 mentioned this pull request Mar 3, 2017

Updated makefile to use dmd/generated/os/release/model/dmd dlang/dlang.org#1598

Merged

This was referenced Mar 8, 2017

Unify path delimiters in win32.mak #6602

Closed

Fix win64.mak system #6620

Merged

wilzbach mentioned this pull request Sep 14, 2017

ddmd -> dmd transition #7135

Merged

15 tasks

wilzbach mentioned this pull request Dec 12, 2017

Make C++ warnings errors (i.e. -Werror) in posix.mak #7387

Merged

Generate all .o .deps and binaries to generated/ #6562

Generate all .o .deps and binaries to generated/ #6562

Conversation

RazvanN7 commented Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RazvanN7 commented Feb 23, 2017

UplinkCoder commented Feb 23, 2017

wilzbach commented Feb 23, 2017

andralex commented Feb 24, 2017

skl131313 commented Feb 24, 2017

MartinNowak commented Feb 25, 2017

MartinNowak commented Feb 25, 2017

skl131313 commented Feb 25, 2017 • edited Loading

RazvanN7 commented Feb 27, 2017

CyberShadow commented Feb 27, 2017 • edited Loading

RazvanN7 commented Feb 27, 2017

andralex commented Feb 27, 2017

CyberShadow commented Feb 27, 2017

CyberShadow commented Feb 27, 2017

andralex commented Feb 27, 2017

CyberShadow commented Feb 27, 2017 • edited Loading

wilzbach commented Feb 27, 2017

RazvanN7 commented Feb 27, 2017

CyberShadow commented Feb 27, 2017

CyberShadow commented Feb 27, 2017

CyberShadow commented Feb 27, 2017

CyberShadow commented Feb 27, 2017 • edited Loading

MartinNowak commented Feb 27, 2017

MartinNowak commented Mar 2, 2017

CyberShadow commented Mar 2, 2017

RazvanN7 commented Mar 2, 2017 • edited Loading

MartinNowak commented Mar 2, 2017

MartinNowak commented Mar 2, 2017

CyberShadow commented Mar 2, 2017

MartinNowak commented Mar 2, 2017

MartinNowak commented Mar 3, 2017

wilzbach commented Mar 4, 2017

RazvanN7 commented Feb 23, 2017 •

edited

Loading

skl131313 commented Feb 25, 2017 •

edited

Loading

CyberShadow commented Feb 27, 2017 •

edited

Loading

CyberShadow commented Feb 27, 2017 •

edited

Loading

CyberShadow commented Feb 27, 2017 •

edited

Loading

RazvanN7 commented Mar 2, 2017 •

edited

Loading