Skip to content
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

Fix Issue 20244 - New dmd option -preview=noXlinker does not work on Linux to build a simple D application #10438

Merged
merged 1 commit into from Sep 26, 2019

Conversation

JinShil
Copy link
Contributor

@JinShil JinShil commented Sep 26, 2019

No description provided.

@dlang-bot
Copy link
Contributor

dlang-bot commented Sep 26, 2019

Thanks for your pull request and interest in making D better, @JinShil! 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 coverage diff by visiting the details link of the codecov check)
  • 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

Auto-close Bugzilla Severity Description
20244 normal New dmd option -preview=noXlinker does not work on Linux to build a simple D application

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

@Geod24
Copy link
Member

Geod24 commented Sep 26, 2019

Stable ?

Actually this isn't part of a release, right ?

@JinShil
Copy link
Contributor Author

JinShil commented Sep 26, 2019

Stable ?

Yes will do.

@JinShil JinShil changed the base branch from master to stable September 26, 2019 05:11
@JinShil JinShil changed the base branch from stable to master September 26, 2019 05:12
@JinShil
Copy link
Contributor Author

JinShil commented Sep 26, 2019

Actually, the -preview=noXlinker option doesn't appear to be in stable yet, so this should go to master for now.

This PR is ready to go.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 26, 2019

Arg! Sorry @thewilsonator . It really is ready now.

@CyberShadow
Copy link
Member

Sounds like the change enabled by this preview flag is going to make it impossible to write a dmd.conf that will work across two DMD versions? I think that would be a blocker for enabling it by default.

@CyberShadow
Copy link
Member

IMO a new compiler flag for this rare use case (sending options to the linker driver) would be much more preferable to the breaking change. Also, I should note that there exist solutions to this problem which don't require any change in the compiler at all - simply write your own linker driver which adds the necessary switches, and get DMD to invoke it. This is what Digger has been doing when working around certain problems with building some D versions on some systems.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 26, 2019

The reason I chose not to implement -Xcc is because we have too many ways of doing the same thing. The code is becoming difficult to learn and maintain as we try to carry forward the design decisions of the past indefinitely. I'm sure you and others will disagree, but I think it is best to invest with the hope that someday we can delete legacy code, not continue to add to it.

If and when -preview=noXlinker becomes the default, a -revert=noXlinker will also be added. I don't think it is too much to expect existing users that don't wish to update their existing build scripts to simply add -revert=noXlinker to their dmd.conf file.

@dlang-bot dlang-bot merged commit de06832 into dlang:master Sep 26, 2019
@CyberShadow
Copy link
Member

CyberShadow commented Sep 26, 2019

and maintain as we try to carry forward the design decisions of the past indefinitely

You could say that about any breaking change. (Edit: deleted unconstructive rant)

If and when -preview=noXlinker becomes the default, a -revert=noXlinker will also be added.

That's a lot of compiler switches. -Xcc would be one fewer and 1. Not break scripts 2. Not require editing dmd.conf in a backwards-incompatible way.

To elaborate on the dmd.conf problem - /etc/dmd.conf is generally one of these three:

  1. It was installed manually by the user, e.g. if they installed DMD from a tarball.
  2. It was installed by a package manager, but the user has edited it since installation.
  3. It was installed by a package manager, and the user has not edited it since installation.

In cases 1 and 2, if issue 20244 is anything typical for Linux systems, upgrading DMD is going to break it completely. Right? And, if you fix dmd.conf, you can't downgrade / use older DMD versions again. I hope you will agree that this isn't acceptable.

@JinShil JinShil deleted the fix_noxlinker branch September 26, 2019 06:33
@JinShil
Copy link
Contributor Author

JinShil commented Sep 26, 2019

That's a lot of compiler switches.

In the long term both the -preview and -revert switches will be removed.

@JinShil
Copy link
Contributor Author

JinShil commented Sep 26, 2019

You are breaking things for their own sake

No, I'm not. I'm trying to fix problems because no one else is. And I don't appreciate being lectured in such a way from spectators.

In cases 1 and 2, if issue 20244 is anything typical for Linux systems, upgrading DMD is going to break it completely. Right? And, if you fix dmd.conf, you can't downgrade / use older DMD versions again. I hope you will agree that this isn't acceptable.

Agreed. I will look for a different solution and perhaps revert -preview=noXlinker.

@CyberShadow
Copy link
Member

No, I'm not. I'm trying to fix problems because no one else is. And I don't appreciate being lectured in such a way from spectators.

Color me an arrogant idiot, but here is what this spectator has observed. Within the last few months, you undertook three projects which you seemed to be completely confident that you were on the right path until it turned out you weren't:

  1. Making the rt package from Druntime public
  2. Deprecating the in keyword in function signatures and removing its use throughout Phobos
  3. Changing the semantics of -L in a way that breaks scripts and dmd.conf on Linux

What I'm bothered by is that every time this involved some length of discussion throughout which you were adamant that this was the way to go. I'm sure it can all be attributed to failures in communication, but... well, I'm not sure what to say. Take this particular case: I said nothing new in my last comment, merely elaborated on the problem which I pointed out in my first comment.

Nevertheless I'm glad we are in agreement once again.

@airwin1
Copy link

airwin1 commented Sep 26, 2019

I am no dmd expert. But as a happy user of this new feature I did notice the flag was called -preview=noXlinker, and that "preview" is a strong indication to me that it is something entirely new with some barriers to entry. So I find it reasonably acceptable that it is necessary for the user to edit the -L options in the dmd configuration file to get this new feature to work properly right now. And I don't think there is a distro problem since the user of this feature can simply copy the default configuration file to a file he can edit, and then specify that file location with the -conf option. And if a distro becomes an early adopter as a whole, they can certainly put the -preview=noXlinker option in the default dmd configuration file and adjust the -L options in that file for the new -L semantics. So I think this is a good gentle way to introduce this new feature, but assuming after a lot of experience most like these new -L semantics as much as I do, I also like the fact that the plan is this feature will become the default. So hang in there JinShil, and stick to your guns.

@CyberShadow
Copy link
Member

Another aspect of changing the meaning of a linking-related compiler flag is that GDC and LDC frontends (ldmd and gdmd) will need to adjust as well. Probably GDC and LDC maintainers should be included in the discussion.

But as a happy user of this new feature

What do you use it for?

Perhaps DMD should just have -static, to mirror the gcc flag.

@airwin1
Copy link

airwin1 commented Sep 26, 2019

There are something like ~5 dmd bug reports concerning getting access to the C compiler options, and if you read through those the need is for a lot more than just -static. For example, in my case (linking a mixture of C and D code from various static libraries and a D example) I need that access for -pthread (a gcc compiler option [and I think some other compilers as well] that allows gcc to decide on exactly how to link the thread library). For my particular software project http://plplot.org I have noticed there are at least 5 independent external libraries that need to link to the thread library. So that is evidence that the need for the -pthread option is quite common in free software projects. So it is also going to be a common problem for dmd if any of the many free software libraries that are built with -pthread attempt to create a D binding for the static case (as we do).

I notice the weight of your continued complaints did help to convince JinShil to revert his work which is not good from my perspective since it means I cannot use dmd to build the D binding to PLplot in the static case. But my hope is he will bounce back with an implementation of the -Xcc option for dmd since that would work for me as well and also satisfy the many dmd bug reports concerning the issue of access to the C compiler options.

@CyberShadow
Copy link
Member

CyberShadow commented Sep 26, 2019

since it means I cannot use dmd to build the D binding to PLplot in the static case

Would this achieve your goal:

  1. Create an executable script file with the contents #!/bin/sh / exec cc -static "$@" (add flags as needed here)
  2. Run DMD as CC=path/to/above/script dmd ...

?

Edit: actually, it's not even necessary to create a file, this should also work:

CC='cc -static' dmd ...

Though, it does not allow appending options, only prepending them.

@Geod24
Copy link
Member

Geod24 commented Sep 26, 2019

Would this achieve your goal:

Doesn't compose well, e.g. with LDFLAGS / lflags (dub). Good luck dealing with two or more libraries providing their own wrapper in a cross-platform manner.

Perhaps DMD should just have -static, to mirror the gcc flag.

With this approach we'd be condemned to always follow what the underlying driver offers.
The approach @JinShil is using actually seems like the "right" solution, as in, it's not limiting people and potentially putting more work on our table for as long as the underlying driver can change.

I share your concerns about breaking code, but the attitude I've witnessed from most users was that they were willing to accept a change that had an upgrade path. E.g. deprecated have been a huge win for the language and allowed us to move forward on many topics.
And the landscape for languages have changed a lot over the past decade: Rust and Go are increasingly popular, despite being very unstable (not only the language, but the ecosystem).
Even C++ recognized the need for "fast" iteration cycles.

While I haven't always agreed personally with all the changes coming, I'm glad @JinShil challenged some of the features / designs we have. This led, for example, to the deprecation of D1-style operator overloads, which weren't on anyone's radar AFAICS.

If anything, we need more people challenging and discussing changes and their implications (like you just did), instead of people blindly merging pull requests.

@CyberShadow
Copy link
Member

Doesn't compose well, e.g. with LDFLAGS / lflags (dub). Good luck dealing with two or more libraries providing their own wrapper in a cross-platform manner.

It doesn't sound to me like libraries should have much control over how the applications they're used in will be linked. If we allow libraries' dub.sdl files to add linking flags arbitrarily, wouldn't that result in clashes anyway?

The approach @JinShil is using actually seems like the "right" solution, as in, it's not limiting people and potentially putting more work on our table for as long as the underlying driver can change.

Then, what's the problem with -Xcc?

I share your concerns about breaking code, but the attitude I've witnessed from most users was that they were willing to accept a change that had an upgrade path. E.g. deprecated have been a huge win for the language and allowed us to move forward on many topics.

Deprecation allows supporting a range of compiler versions, and when it's introduced, code still continues to work with a warning. A backwards-incompatible change to the meaning of a pre-existing flag will not have the same effect. To remedy this, first, -preview=noXlinker would need to continue working as a no-op after it's made a default, and/or -revert=noXlinker would need to be added now as a no-op; then, dmd.conf files and linking scripts would need to have one of these added ahead of time for a smooth transition. Deprecation also is generally not done for features that are very frequently used in D code; breaking compatibility with current dmd.conf files doesn't seem like that either.

@Geod24
Copy link
Member

Geod24 commented Sep 26, 2019

To remedy this, first, -preview=noXlinker would need to continue working as a no-op after it's made a default, and/or -revert=noXlinker would need to be added now as a no-op; then, dmd.conf files and linking scripts would need to have one of these added ahead of time for a smooth transition.

That sounds correct, yes. Just to be clear, are you recommending we should go with that, or is that approach problematic ?

breaking compatibility with current dmd.conf files doesn't seem like that either.

Any release that removes a flag could do that, since the default dmd.conf is often not under our control. They are rare, but they do happen.

It doesn't sound to me like libraries should have much control over how the applications they're used in will be linked.

I don't see how that's workable. Bindings for example have to control how the application they're used in are linked.

@CyberShadow
Copy link
Member

CyberShadow commented Sep 26, 2019

That sounds correct, yes. Just to be clear, are you recommending we should go with that, or is that approach problematic ?

In my opinion, a new switch would be less trouble for everyone, but maybe we should explore the problem a bit more.

Any release that removes a flag could do that, since the default dmd.conf is often not under our control. They are rare, but they do happen.

True; however, this would be different because it would break versions of dmd.conf that we (and other parties distributing DMD) have shipped. I don't think that's happened in a long time, and certainly not at all often.

I don't see how that's workable. Bindings for example have to control how the application they're used in are linked.

Would it be wrong of me to say that the great majority of bindings simply require linking to the appropriate library? Such as using -lfoo. We also have pragma(lib, "foo"), though it doesn't work with all object file formats.

@airwin1
Copy link

airwin1 commented Sep 26, 2019

Deprecation also is generally not done for features that are very frequently used in D code; breaking compatibility with current dmd.conf files doesn't seem like that either.

Sorry, but we are going to have to disagree on this one. There is a whole spectrum of attitudes toward welcoming change and adjusting to it, and it appears we are far divided on that.

I do agree that changing the -L semantics is a big change. So arguing about whether that change in semantics is an improvement or not is well justified. I happen to think it is an improvement over the "spot rezone" approach with -Xcc since it just makes sense to me to always communicate with the underlying compiler when dealing with linking options because compilers are more powerful linkers (e.g., the -pthread option, but there is quite a few more) than ld. And I think most of the other commentators on these new semantics think it is a well merited change as well.

But I don' t think you are arguing about the merits of the new semantics but instead you are concerned about the potential pain to users of this big a change to the -L semantics. And here I disagree again because this early introduction has been gentle (no breakage once the bug I found was fixed except that early adopters of this new feature have to adjust the -L usage in the dmd configuration file they have decided to use). I am also confident that the much later move to adopting these new semantics as default while deprecating the old semantics can be handled smoothly (i.e, with minimal pain to users) as well (e.g., by documenting this change in the semantics in the release notes, updating the man pages and --help results, and by modifying the -L options in the default dmd configuration file according to the new default semantics). But I suspect you don't share my confidence. :-)

@CyberShadow
Copy link
Member

Here is what bothers me about the new behavior itself subjectively:

  1. It makes sense to me that -L on DMD's command line does the same thing as -L on gcc's command line:
$ dmd -L-lz test.d
$ gcc -L-lz test.c

These are roughly equivalents of the same thing ("pass this flag to the linker"). Even though cc will also understand -L, and pass it to the linker, it is not the same thing.

  1. -L will now become unreasonably overloaded. For instance, -L-L-L/usr/lib will now be a valid DMD switch, which will:

    1. Indicate to DMD an option for the linker driver (-L-L/usr/lib)
    2. Indicate to the linker driver an option for the linker (-L/usr/lib)
    3. Indicate to the linker a path where to look for libraries (/usr/lib).

When I was just starting with POSIX compilation toolchains many years ago, I remember finding -L-L already very confusing.

except that early adopters of this new feature have to adjust the -L usage in the dmd configuration file they have decided to use

I think this is a serious aspect of this that you are glossing over. For example some people need to have several DMD versions installed side-by-side; version managers are commonly used with DMD. Of course, users can maintain custom dmd.conf files for every dmd version they need, but we should avoid complications if possible.

There is also the issue with non-DMD compilers I mentioned above. For instance, GDC probably does not invoke cc to do the linking. If anything, I would defer this decision to the GDC/LDC maintainers (they are also much more familiar with this problem space than I am). @ibuclaw @kinke

@ibuclaw
Copy link
Member

ibuclaw commented Sep 26, 2019

These are roughly equivalents of the same thing ("pass this flag to the linker"). Even though cc will also understand -L, and pass it to the linker, it is not the same thing.

I beg your pardon? This is from the gcc manpage.

-Ldir
    Add directory dir to the list of directories to be searched for
    -l.

dmd -L has never done the same thing as gcc.

@kinke
Copy link
Contributor

kinke commented Sep 26, 2019

I already laid out my view on this in the original PR - in short, I'd prefer DMD to adopt -Xcc, which LDC has for 1 or 2 years now, to avoid the breakage and have 2 distinct switches for linker and linker driver (i.e., C compiler; no such thing for Windows). Note that LDC forwards some -L values to the C compiler (not to the actual linker), such as -L-L<dir> and -L-l<lib> (=> gcc/clang -L<dir> -l<lib>), because that makes a difference AFAIK wrt. libs lookup, and obviously -L-Wl,<arg1>,<arg2>,.... That scheme of detecting some -L values turned out to be insufficient (e.g., we have a -static option, which would also be expressible as -Xcc=-static nowadays), so -Xcc was introduced, no breaking changes whatsoever, just more flexibility.

A difficulty wrt. -Xcc is the linker cmdline order - ldc2 -LldArg1 -Xcc=gccArg -LldArg2 results in gcc -Xlinker ldArg1 gccArg -Xlinker ldArg2.

That whole Posix mess (abuse C compiler as linker because it's preconfigured for accompanying C libs, startup object files etc.) is the reason LDC's -link-internally (use integrated LLD for linking) isn't really usable for non-Windows targets (you'd need a huge list of linker cmdline options, i.e., the ones injected by gcc).

@jpf91
Copy link
Contributor

jpf91 commented Sep 26, 2019

There is also the issue with non-DMD compilers I mentioned above. For instance, GDC probably does not invoke cc to do the linking.

GDC is a special case here anyway. We simply reuse the GCC code for linking, which means gdc will behave exactly as gcc does with regards to linker flags. But technically, we never call gcc, we just have exactly the same implementation / share the source code for linker flag handling.

@CyberShadow
Copy link
Member

CyberShadow commented Sep 26, 2019 via email

@airwin1
Copy link

airwin1 commented Sep 26, 2019

I have already expressed myself in quite general terms about dealing with change. So I plan to say no more on that topic. And I don't know much about dmd so this is likely my last post on this whole topic. However, I do want to take this opportunity to to thank @kinke for implementing the ldc2 -Xcc option which I am already using for my PLplot -pthread needs. So if someone wants to implement the -Xcc option for dmd OR bring back the -review=noXlinker option that worked momentarily for me until it was withdrawn again, that would be fine with me. But please do one or the other and do not leave dmd in its present state (unusable for PLplot for the static libraries case on at least the Linux platform because there is no way to communicate with the underlying C compiler that dmd uses for linking).

@CyberShadow
Copy link
Member

OR bring back the -review=noXlinker option that worked momentarily for me until it was withdrawn again

#10439 is still open.

which I am already using for my PLplot -pthread needs

Have you tried 'CC=cc -pthread' dmd ...? Does it work?

@CyberShadow
Copy link
Member

CyberShadow commented Sep 26, 2019

So if someone wants to implement the -Xcc option for dmd

Here is a draft:

master...CyberShadow:pull-20190926-222551

Any thoughts on documenting and submitting that as a PR as a replacement for noXlinker?

Edit: Actually, that's what draft PRs are for, submitted: #10441

@airwin1
Copy link

airwin1 commented Sep 26, 2019

#10439 is still open.

To be clear that PR is a revert of all of -JinShil's work on -review=noXlinker. And that PR is judged ready to be merged so I have assumed that will happen automatically, but I would be happy to be proved wrong for the reason I expressed in my last post. Enuf said by me as you guys work this issue out.

@CyberShadow
Copy link
Member

@airwin1 Since you have a use case in practice, it would be really helpful if you could continue to work with us to ensure that whatever we come up with works for you. I.e. it would be great if you could check that #10441 helps you achieve the same goal, and tell us what's missing or how it is more helpful than setting the CC environment variable.

@CyberShadow
Copy link
Member

And that PR is judged ready to be merged

Fixed.

@airwin1
Copy link

airwin1 commented Sep 27, 2019

@CyberShadow > [I]it would be great if you could check that #10441 helps you achieve the same goal, and tell us what's missing.

Will do. Is #10441 ready for immediate testing, and if so how exactly would you communicate the -pthread option to the C compiler with it?

Also, I don't use github except to apply patches from there to a git working directory on my home computer. So is there a convenient way for me to create one giant patch representing your PR that I can apply to current git master HEAD or should I just apply your work with the two patches representing the two separate commits in your PR? (This question is not that urgent when your PR consists of just two commits, but it might become much more urgent if that expands to lots of commits.)

Also, I should mention that my use case is a work in progress that is only completely tested (on Linux for a version of PLplot with all components configured for that platform) for gdc, and only tested on that platform for ldc2 with -Xcc and dmd with the current -preview=noXlinker option on the dmd git master branch HEAD for an incomplete version of PLplot with just the D components and one threaded component configured beyond the core C components. (I also have a good preliminary report for a "hello, world" test_d project without -pthread needs for dmd on Darwin (MacPorts)). So everything is looking pretty promising, but I (and the tester with access to MacPorts) still have a lot of work to do to get testing of a completely configured PLplot working on both Linux (with gdc, ldc2, and dmd) and Darwin (MacPorts with dmd). And once all problems on those platforms are fixed, I also have a Windows (MSYS2) tester standing by who is willing to try building dmd on MSYS2 (since that Windows distribution of free software does not supply it) to see if the D components of PLplot work on that platform (which they currently do for virtually all other PLplot components other than D).

In sum, it is all looking quite promising, but there is a lot of work for me (and testers on MacPorts and MSYS2) to do until we have a good cross-platform D solution for a fully configured PLplot implemented. Meanwhile, I should be able to do testing of the incomplete PLplot version described above. And I plan to continue such testing (and also testing of ldc2) as your PR matures and our build-system configuration of threading and our D component matures.

But I really must greatly reduce my commenting here so I can get to grips with all the PLplot work I have just promised to do. :-)

@CyberShadow
Copy link
Member

Is #10441 ready for immediate testing

I think so, yes.

how exactly would you communicate the -pthread option to the C compiler with it?

-Xcc=-pthread

So is there a convenient way for me to create one giant patch representing your PR that I can apply to current git master HEAD

Yes! https://patch-diff.githubusercontent.com/raw/dlang/dmd/pull/10441.diff contains the condensed diff for all commits in the PR.

or should I just apply your work with the two patches representing the two separate commits in your PR?

You may find it easier to check out the PR branch.

$ cd .../dlang/dmd
$ git remote add --fetch CyberShadow https://github.com/CyberShadow/dmd
$ git checkout pull-20190926-222551

Also, if I may plug a personal project - you can get, build and run DMD + dependencies using Digger with one command:

$ digger run master+dmd#10441 -- dmd -Xcc=-pthread ...

Also, I should mention that my use case is a work in progress that is only completely tested (on Linux for a version of PLplot with all components configured for that platform) for gdc, and only tested on that platform for ldc2 with -Xcc and dmd with the current -preview=noXlinker option on the dmd git master branch HEAD for an incomplete version of PLplot with just the D components and one threaded component configured beyond the core C components. (I also have a good preliminary report for a "hello, world" test_d project without -pthread needs for dmd on Darwin (MacPorts)). So everything is looking pretty promising, but I (and the tester with access to MacPorts) still have a lot of work to do to get testing of a completely configured PLplot working on both Linux (with gdc, ldc2, and dmd) and Darwin (MacPorts with dmd). And once all problems on those platforms are fixed, I also have a Windows (MSYS2) tester standing by who is willing to try building dmd on MSYS2 (since that Windows distribution of free software does not supply it) to see if the D components of PLplot work on that platform (which they currently do for virtually all other PLplot components other than D).

That sounds pretty great. I'm not sure what our situation with MSYS2 or other POSIX environments on Windows is right now though.

@airwin1
Copy link

airwin1 commented Sep 29, 2019

I have had some initial testing success with #10441, and see that PR for more details concerning those test results and an easy way for you to replicate them for yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants