-
-
Notifications
You must be signed in to change notification settings - Fork 411
Partial fix for running druntime unittest with PIC=1. #1721
Conversation
| GENERATED:=./generated | ||
| ROOT:=$(GENERATED)/$(OS)/$(BUILD)/$(MODEL) | ||
|
|
||
| OPTIONAL_PIC:=$(if $(PIC),-fPIC,) |
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'd prefer to explicitly pass OPTIONAL_PIC=$(OPTIONAL_PIC) from the top makefile as is done for the other variables above.
See
Lines 264 to 266 in 4af3423
| $(QUIET)$(MAKE) -C test/$* MODEL=$(MODEL) OS=$(OS) DMD=$(abspath $(DMD)) BUILD=$(BUILD) \ | |
| DRUNTIME=$(abspath $(DRUNTIME)) DRUNTIMESO=$(abspath $(DRUNTIMESO)) LINKDL=$(LINKDL) \ | |
| QUIET=$(QUIET) TIMELIMIT='$(TIMELIMIT)' |
The fact that explict CLI arguments (like
PIC=1) are passed to proper submake invocations is fairly subtle to rely on.
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.
ditto
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. I just added it as an argument.
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.
@jmdavis why not pass OPTIONAL_PIC (as @MartinNowak suggested), instead of PIC, so the OPTIONAL_PIC doesn't need to be calculated again?
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 thought that I had done what was requested, but I could have misunderstood, and my makefile-foo is quite weak.
b884848 to
578aa83
Compare
|
Rebased. |
|
With my comment applied, or as is, I think we should merge this. I've installed Arch today and spent an hour trying to figure out why druntime unittests are failing - it lead to #1863, but I'll close it now as a duplicate of this. cc @MartinNowak @andralex IMHO, it's a simple bugfix and it's staled from December. |
|
Also needs rebase now :( |
This fixes it so that the builds in the test folder are built with -fPIC when the unittest build is run with PIC=1. Unfortunately, the line_trace test in test/exceptions is then failing (at least on my system), because it doesn't actually generate a stacktrace. So, this doesn't seem to completely fix it so that the PIC=1 unittest build works, but it's close.
|
Thanks for your pull request, @jmdavis! 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. |
Rebased. |
|
Btw as |
This fixes it so that the builds in the test folder are built with
-fPIC when the unittest build is run with PIC=1. Unfortunately, the
line_trace test in test/exceptions is then failing (at least on my
system), because it doesn't actually generate a stacktrace. So, this
doesn't seem to completely fix it so that the PIC=1 unittest build
works, but it's close.