Fix import from rt.config #2032

belka-ew · 2018-01-05T07:02:12Z

src/rt isn't shipped with druntime's imports, so it can't be
imported directly from core.runtime. This PR fixes importing rt.config
from a public druntime module by replacing it with an extern
definition.

To reproduce the bug create a file that imports 'core.runtime' and
compile it with:
dmd -unittest -deps filename.d

The error is:
/usr/include/dmd/druntime/import/core/runtime.d(653): Error: module
config is in file 'rt/config.d' which cannot be read

dlang-bot · 2018-01-05T07:02:12Z

Thanks for your pull request, @belka-ew! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	18193	module config is in file 'rt/config.d' which cannot be read

rainers · 2018-01-05T08:47:13Z

Thanks. Happens even without -unittest.

Looks like a regression (-deps has changed it's meaning, unfortunately, but having the import in a core module is incorrect anyway) so should be better targeting the stable branch. A bugzilla report should also be added so it appears in the changelog.

belka-ew · 2018-01-05T09:03:55Z

Looks like a regression (-deps has changed it's meaning, unfortunately, but having the import in a core module is incorrect anyway) so should be better targeting the stable branch.

Should I close this PR then?

It was introduced by d757a7d.

rainers · 2018-01-05T09:20:13Z

Should I close this PR then?

No, it should be switched to target "stable" (click "Edit" for the title) and a bugzilla entry should be added at https://issues.dlang.org/

belka-ew · 2018-01-05T10:10:53Z

It seems I messed it up. My branch is created from master. GitHub web interface allows to change the target branch only, not to rebase on another branch.

Burgos · 2018-01-05T10:11:48Z

You can just rebase locally your branch on stable and push the new state to your remote and this PR will be in a good shape again :-)

… be read (edit) src/rt isn't shipped with druntime's imports, so it can't be imported directly from core.runtime. This PR fixes importing rt.config from a public druntime module by replacing it with an extern definition. To reproduce the bug create a file that imports 'core.runtime' and compile it with: dmd -unittest -deps filename.d The error is: /usr/include/dmd/druntime/import/core/runtime.d(653): Error: module config is in file 'rt/config.d' which cannot be read

belka-ew · 2018-01-05T10:18:52Z

Fine. That looks a bit better. @rainers and @Burgos, thanks.

PetarKirov

Thanks, LGTM.

wilzbach · 2018-01-05T13:20:35Z

FYI: there are two more imports:

druntime/src/core/thread.d

Lines 713 to 718 in 7bfaa8c

    
           version (Shared) 
        
           { 
        
               import rt.sections; 
        
               auto libs = pinLoadedLibraries(); 
        
               auto ps = cast(void**).malloc(2 * size_t.sizeof); 
        
               if (ps is null) onOutOfMemoryError();

druntime/src/core/thread.d

Lines 305 to 309 in 7bfaa8c

    
           version (Shared) 
        
           { 
        
               import rt.sections; 
        
               Thread obj = cast(Thread)(cast(void**)arg)[0]; 
        
               auto loadedLibraries = (cast(void**)arg)[1];

wilzbach · 2018-01-05T13:24:05Z

Also this should have been accompanied by a unittest, s.t. the regression can't happen again.
@belka-ew could you quickly add a test? (see /test)

wilzbach · 2018-01-05T13:25:58Z

Also @ other reviewers. This would have never been merged, because for auto-tester new users need to be approved. So if auto-tester doesn't show up in the list of CIs, head over and you will see this screen:

@belka-ew I just clicked on "Approve" ;-)

belka-ew · 2018-01-05T13:44:42Z

I'll add a test this week. Can't promise to do it today.
Should I fix other imports in this PR?
What kind of test would it be, I see several directories in "test"?

wilzbach · 2018-01-05T13:50:04Z

Should I fix other imports in this PR?

If you ask like this, I can't really say no ;-)
But it's not a requirement for this PR.

What kind of test would it be, I see several directories in "test"?

You probably have to add a new directory to test.

belka-ew · 2018-01-06T07:07:00Z

/home/circleci/dmd/generated/linux/release/64/dmd -m64 -fPIC -w -I../../src -I../../import -Isrc -defaultlib= -debuglib= -dip1000 -L/home/circleci/druntime/generated/linux/debug/64/libdruntime.a -g -debug -ofgenerated/linux/debug/64/fiber_guard_page src/fiber_guard_page.d

There seems to be a very generic problem with druntime tests. -I../../src is always passed when compiling. ../../src contains both, core and rt, it prevents such errors from being found.

It is just a note, I still need some time to experiment with the test runner.

belka-ew · 2018-01-07T20:17:46Z

I couldn't find a better way to write the test. Is it ok? If so, I'll try to fix other two imports.

wilzbach

Thanks. Looks good! Not so happy about the symlinks though.

wilzbach · 2018-01-09T00:12:46Z

test/imports/src/core

@@ -0,0 +1 @@
+../../../src/core


Hmm, why do you need these files?
dmd isn't used with -conf= and will use the import path of druntime. If you don't trust it, you can always add -I../../import?

It fails: https://circleci.com/gh/dlang/druntime/1546

Testing bug18193
/home/circleci/dmd/generated/linux/release/64/dmd -o- -deps=generated/linux/debug/64/bug18193.done -Isrc -I../../import src/bug18193
Error: cannot find source code for runtime library file 'object.d'
dmd might not be correctly installed. Run 'dmd -man' for installation instructions.
config file: /home/circleci/dmd/generated/linux/release/64/dmd.conf

belka-ew · 2018-01-09T08:25:06Z

The fix for the second part:
https://github.com/belka-ew/druntime/compare/fix-deps-rt-import...belka-ew:fix-deps-rt-import2

rainers · 2018-01-09T08:30:55Z

The problem seems to be that the tests are run without the import files being copied over from the source directory. That's why the other tests specify -I../../src, too.
Not sure if it is specific to the circleci script, but the default target should be built before running the tests.

belka-ew · 2018-01-11T09:02:33Z

Any chance to get it merged with or without the tests before 2.080.1? As said it is a general problem with the makefiles, which prevents such bugs from being found. And this problem is not related to this regression introduced in 2.080.0 and can be fixed in another PR. I can create an issue if needed.

wilzbach · 2018-01-11T18:38:03Z

I added 3d153f8 to your PR. I think that should solve the problem. Sorry for the delay.

wilzbach · 2018-01-11T19:21:30Z

Looks all good. @belka-ew do you mind if I squash your last three commits into one? No need to keep the experiments in the history.

belka-ew · 2018-01-12T08:11:54Z

@wilzbach, thanks for your fast response. I squashed the commits and added the fixes for core.thread.

belka-ew · 2018-01-12T09:02:59Z

I assume that the tests fail because code.dlang.org is dead?

wilzbach · 2018-01-12T13:21:56Z

No the DUB registry outage was only for two hours and we have mirrors:

Jenkins failure

Sending interrupt signal to process

./travis-ci.sh: line 40: 29145 Terminated              dub build --compiler=$DC --override-config=vibe-d:core/$VIBED_DRIVER $DUB_ARGS

script returned exit code 143

Might be related to: dlang/ci#118

DAutoTest failure

.generated/stable_dmd-2.072.2/dmd2/linux/bin64/dub build --single --compiler=.generated/stable_dmd-2.072.2/dmd2/linux/bin64/dmd assert_writeln_magic.d
Root package assert_writeln_magic references unknown package libdparse
posix.mak:818: recipe for target '.generated/assert_writeln_magic' failed

2.072.0 doesn't come with mirrors, will look into upgrading it.

In the mean time I will git commit --amend the last commit and force-push as that's the only way to restart DAutoTest.

wilzbach · 2018-01-12T15:22:21Z

2.072.0 doesn't come with mirrors, will look into upgrading it.

I already did so, it's just not in stable yet:

dlang/dlang.org#1926

belka-ew requested a review from MartinNowak as a code owner January 5, 2018 07:02

dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Jan 5, 2018

belka-ew changed the base branch from master to stable January 5, 2018 10:07

PetarKirov approved these changes Jan 5, 2018

View reviewed changes

PetarKirov added the auto-merge label Jan 5, 2018

wilzbach removed the auto-merge label Jan 5, 2018

belka-ew requested review from andralex and wilzbach as code owners January 7, 2018 19:05

wilzbach reviewed Jan 9, 2018

View reviewed changes

Add test to the bug 18193

105d4a5

Require druntime to be built before tests are run

b38f88f

belka-ew requested a review from Burgos as a code owner January 12, 2018 07:59

dlang-bot added the Needs Work label Jan 12, 2018

Fix rt.sections imports in core.thread

515c9e2

dlang-bot removed the Needs Work label Jan 12, 2018

wilzbach approved these changes Jan 12, 2018

View reviewed changes

wilzbach added the auto-merge label Jan 12, 2018

dlang-bot merged commit e5512c4 into dlang:stable Jan 12, 2018

belka-ew deleted the fix-deps-rt-import branch January 12, 2018 19:09

wilzbach mentioned this pull request Jan 18, 2018

Attempt to fix CircleCi #2048

Merged

		@@ -0,0 +1 @@
		../../../src/core No newline at end of file

Uh oh!

Fix import from rt.config #2032

Fix import from rt.config #2032

Uh oh!

Conversation

belka-ew commented Jan 5, 2018

Uh oh!

dlang-bot commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Uh oh!

rainers commented Jan 5, 2018

Uh oh!

belka-ew commented Jan 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Jan 5, 2018

Uh oh!

belka-ew commented Jan 5, 2018

Uh oh!

Burgos commented Jan 5, 2018

Uh oh!

belka-ew commented Jan 5, 2018

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 5, 2018

Uh oh!

wilzbach commented Jan 5, 2018

Uh oh!

wilzbach commented Jan 5, 2018

Uh oh!

belka-ew commented Jan 5, 2018

Uh oh!

wilzbach commented Jan 5, 2018

Uh oh!

belka-ew commented Jan 6, 2018

Uh oh!

belka-ew commented Jan 7, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

belka-ew Jan 9, 2018

Choose a reason for hiding this comment

Uh oh!

belka-ew commented Jan 9, 2018

Uh oh!

rainers commented Jan 9, 2018

Uh oh!

belka-ew commented Jan 11, 2018

Uh oh!

wilzbach commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jan 11, 2018

Uh oh!

belka-ew commented Jan 12, 2018

Uh oh!

belka-ew commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Jan 12, 2018

Uh oh!

wilzbach commented Jan 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dlang-bot commented Jan 5, 2018 •

edited

Loading

belka-ew commented Jan 5, 2018 •

edited

Loading

wilzbach commented Jan 11, 2018 •

edited

Loading

belka-ew commented Jan 12, 2018 •

edited

Loading