Skip to content

Comments

Delete dmd/src/dmd#6595

Closed
RazvanN7 wants to merge 1 commit intodlang:masterfrom
RazvanN7:Remove_dmd
Closed

Delete dmd/src/dmd#6595
RazvanN7 wants to merge 1 commit intodlang:masterfrom
RazvanN7:Remove_dmd

Conversation

@RazvanN7
Copy link
Contributor

@RazvanN7 RazvanN7 commented Mar 6, 2017

After modifying all the scrips to use dmd/generated/os/build/model/dmd instead of dmd/src/dmd, we can delete the dmd/src/dmd binary.


dmd: $G/dmd $G/dmd.conf
cp $< .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove the src/dmd.conf target as well? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you were referring to $G/dmd.conf

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 7, 2017

CC @braddr , @CyberShadow , @MartinNowak

@CyberShadow
Copy link
Member

In win32.mak:

G = $(GEN)\$(OS)$(MODEL)

OS is not defined anywhere.

@CyberShadow
Copy link
Member

Also, the directory structure is very different on Windows and POSIX.

@CyberShadow
Copy link
Member

Noting that to reliably use the new paths, one needs to either glob, or either invoke or reimplement osmodel.mak in their code. This is a regression in complexity. Perhaps it would be an improvement to allow specifying the desired output path of the DMD binary as a Makefile parameter.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 7, 2017

@CyberShadow From what I could deduce, the directory hierarchies are very different when it comes to posix vs windows builds. For example, if you look at phobos : the linux version has a generated/os/build/model hierarchiy, while the windows version employs a totally different hierarchy (there is a src folder where all the sources are kept - dmd, druntime, phobos - alltogheter and a bin directory where libraries and binaries are generated). Seeing this inconsistency, I thought that the build chains are considered to be different and as a result the hierarchies are different. I will add in win32.mak the os variable which will be set to windows and the build variable which will be set to release, but I don't think that will make any difference.

@CyberShadow
Copy link
Member

Yep, all true. But if we are to define new hierarchies, might as well make them consistent with themselves, right?

@CyberShadow
Copy link
Member

Addressed issues in comments

How come @MartinNowak is the committer (or author?) of that commit? And was that meant to be an amended commit?

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 7, 2017

@CyberShadow It was intended to be an amended commit, but I have no idea why Martin appeared to coauthor the commit. I rebased and tried to commit again but now it looks like dlang-bot is the coauthor of this commit

@CyberShadow
Copy link
Member

I think you might have rebased/amended too far and included the previous merge commit on master.

@CyberShadow
Copy link
Member

I'd suggest trying to amend with --reset-author.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 7, 2017

@CyberShadow Looking through my commit logs I saw that the first commit on this branch accidentally included --amend, but --reset-author solved the case. Thanks

@CyberShadow
Copy link
Member

OK, though you still should rebase on master. Right now your last commit has two parents, and is a merge commit between master and the tip of PR #6572.

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 8, 2017

@andralex @CyberShadow @wilzbach I think this is ready to be merged. Next stop: ddmd -> dmd

@wilzbach
Copy link
Contributor

wilzbach commented Mar 8, 2017

@RazvanN7 I think sth went wrong during your rebase - have a look at the changes tab

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Mar 8, 2017

@wilzbach It seems that circleci needs a dmd.conf in generated

@andralex
Copy link
Member

andralex commented Mar 9, 2017

Well things are failing for the time being. How do we kickstart this?

@RazvanN7
Copy link
Contributor Author

@MartinNowak After this PR : dlang/druntime#1779 one would expect that the autotester uses dmd in generated/os/release/model. Why isn't that happening?

@RazvanN7
Copy link
Contributor Author

@wilzbach I had to keep the dmd.conf target otherwise circleci and travis would fail.

@andralex
Copy link
Member

andralex commented Mar 13, 2017

@RazvanN7 please take a look at the auto-tester (https://auto-tester.puremagic.com/pull-history.ghtml?projectid=1&repoid=1&pullid=6595), seems to still need src/dmd. E.g. from Darwin_64_64:

../dmd/src/dmd -conf= -c -o- -Isrc -Iimport -Hfimport/core/sync/barrier.di src/core/sync/barrier.d
make: ../dmd/src/dmd: No such file or directory

@andralex
Copy link
Member

@MartinNowak @braddr @CyberShadow @wilzbach summoning the wizards for a puzzling matter with the autotester.

I see the autotester fails on e.g. FreeBSD_32 with:

../dmd/src/dmd -conf= -c -o- -Isrc -Iimport -Hfimport/core/sync/barrier.di src/core/sync/barrier.d
gmake: ../dmd/src/dmd: Command not found
posix.mak:146: recipe for target 'import/core/sync/barrier.di' failed

This looks trivial, enough, except https://github.com/dlang/druntime/blob/master/posix.mak has already been changed to use generated/ instead of src/ for locating the dmd binary, and in fact even the line given in the error message (146) is off by one (should be 147). That suggests the druntime makefile run by the autotester is older than the one in master.

What could cause this?

@rainers
Copy link
Member

rainers commented Mar 13, 2017

What could cause this?

The auto-tester passes it explicitly on the command line: https://github.com/braddr/at-client/blob/master/src/do_build_druntime.sh#L15. Same for phobos.

@andralex
Copy link
Member

@rainers thanks! braddr/at-client#1

@RazvanN7 RazvanN7 changed the title Deleted dmd/src/dmd Delete dmd/src/dmd May 6, 2017
@RazvanN7 RazvanN7 force-pushed the Remove_dmd branch 3 times, most recently from 6db406f to eb006d2 Compare May 6, 2017 11:33
@wilzbach wilzbach mentioned this pull request Sep 14, 2017
15 tasks
@andralex
Copy link
Member

@wilzbach is this related to your recent work?

@andralex
Copy link
Member

@RazvanN7 please rebase so we keep this in good shape until we sort it out

@RazvanN7
Copy link
Contributor Author

I think that we can close this since it is out of sync and @wilzbach 's PR does the same thing and much more.

@RazvanN7 RazvanN7 closed this Oct 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants