-
-
Notifications
You must be signed in to change notification settings - Fork 672
Add different ENABLE + unittest modes to build.d #8418
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
Conversation
|
Thanks for your pull request, @wilzbach! 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. Testing this PR locallyIf 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#8418" |
6f95df5 to
2a9d852
Compare
src/dmd/asttypename.d
Outdated
| import dmd.globals : Loc; | ||
| Expression e = new TypeidExp(Loc.init, null); | ||
| Loc loc; | ||
| Expression e = new TypeidExp(loc, null); |
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.
This file isn't part of the DMD build and I only accidentally ended up testing it.
It still doesn't hurt to fix the build failure though.
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.
What was failing and why?
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.
Lines 4758 to 4764 in 67963b9
| extern (C++) final class TypeidExp : Expression | |
| { | |
| RootObject obj; | |
| extern (D) this(const ref Loc loc, RootObject o) | |
| { | |
| super(loc, TOK.typeid_, __traits(classInstanceSize, TypeidExp)); |
That's why Loc.init doesn't work. It might be a good idea to use auto ref in the constructors.
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.
Loc.initial is used instead.
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.
Thanks!
BTW this looks like a half-finished story -> #8427
.circleci/run.sh
Outdated
|
|
||
| # build dmd, druntime, and phobos | ||
| make -j$N -C src -f posix.mak MODEL=$MODEL HOST_DMD=$DMD BUILD=$BUILD ENABLE_WARNINGS=1 PIC="$PIC" all | ||
| "$RDMD" ./src/build.d MODEL=$MODEL HOST_DMD=$DMD BUILD=$BUILD ENABLE_WARNINGS=1 PIC="$PIC" all |
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.
So we slowly start introducing build.d into more parts of the CI infrastructure.
89070ef to
009a092
Compare
| if [ "$MODEL" == "64" ] ; then | ||
| "$RDMD" ./src/build.d MODEL=$MODEL HOST_DMD=$DMD BUILD=$BUILD ENABLE_WARNINGS=1 PIC="$PIC" all | ||
| else | ||
| make -j$N -C src -f posix.mak MODEL=$MODEL HOST_DMD=$DMD BUILD=$BUILD ENABLE_WARNINGS=1 PIC="$PIC" all |
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.
FYI: MODEL=32 doesn't fully work yet. I will fix this in another PR.
| dObjc = true; | ||
|
|
||
| return env; | ||
| if (env.getDefault("ENABLE_DEBUG", "0") != "0") |
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.
Can we copy the documentation comments for these from the Makefile? Or maybe put them in the help output? We should probably do the same for the targets as well. It's not a requirement for this PR, but eventually it'd be nice.
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.
Also, some of these are only applicable to posix, right?
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.
Also, some of these are only applicable to posix, right?
Yes, actually I'm not sure all of these modes even work on Posix (i.e. dmd still fails the test suite with ENABLE_DEBUG - #7528).
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.
Can we copy the documentation comments for these from the Makefile? Or maybe put them in the help output?
Done.
18a24ef to
b3ed103
Compare
Moving more bits from the
posix.maktobuild.d.Depends on #8417.