cache the generated test main file: dub_test_root.d #2005

foerdi · 2020-08-29T00:48:36Z

these changes prevent dub from rebuilding a project every time it runs a unittest.

dlang-bot · 2020-08-29T00:48:38Z

Thanks for your pull request and interest in making D better, @foerdi! 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.

wilzbach

should come with a test (or at least unittests for newly added code)

changelog/cache-generated-test-config.dd

source/dub/dub.d

wilzbach · 2020-08-29T13:20:45Z

source/dub/dub.d

+					settings.platform.platform.join("."),
+					settings.platform.architecture.join("."),
+					settings.platform.compiler, settings.platform.frontendVersion, lbuildsettings.sourceFiles.length);
+			}


I'm aware the code is old, but I would still move this into a separate method. This way it will be easier to add unittests for it.

Ideally you also move the bits to generate the package name from settings into a function instead of copying them around. This way it will be a lot easier in the future to find and adjust them

I'm aware the code is old, but new additions should try to be better and have a few basic comments for future readers.

mainfile = m_project.rootPackage.path ~ format(".dub/code/%s-%s-%s-%s-%s_%s_%s_dub_test_root.d", test_config, settings.buildType, settings.platform.platform.join("."), settings.platform.architecture.join("."), settings.platform.compiler, settings.platform.frontendVersion, lbuildsettings.sourceFiles.length); }

Be aware this is not the .dub/build folder. In this place I cannot access the build_id witch is generated in:

dub/source/dub/generators/build.d

Line 330 in 3288bbb

private string computeBuildID(string config, in BuildSettings buildsettings, GeneratorSettings settings)

The buildsettings can be changed in a project with dependencies.

Maybe someone has a better idea? I am aware this .dub/code solution is not ideal.

In this place I cannot access the build_id witch is generated in:

What's stopping you from moving this out into a new public function:

string computeBuildName(string config, GeneratorSettings settings) { return format("%s-%s-%s-%s-%s_%s-%s", config, settings.buildType, settings.platform.platform.join("."), settings.platform.architecture.join("."), settings.platform.compiler, settings.platform.frontendVersion, hashstr); }

And calling this from both places?

Furthermore, you would want to hash lbuildsettings.sourceFiles - not their length.

Furthermore, you would want to hash lbuildsettings.sourceFiles - not their length.

This is a good idea, thanks.

source/dub/dub.d

test/cache-generated-test-config/test.log

test/cache-generated-test-config.sh

wilzbach · 2020-08-29T15:36:33Z

source/dub/dub.d

+					settings.platform.platform.join("."),
+					settings.platform.architecture.join("."),
+					settings.platform.compiler, settings.platform.frontendVersion, lbuildsettings.sourceFiles.length);
+			}


In this place I cannot access the build_id witch is generated in:

What's stopping you from moving this out into a new public function:

string computeBuildName(string config, GeneratorSettings settings) { return format("%s-%s-%s-%s-%s_%s-%s", config, settings.buildType, settings.platform.platform.join("."), settings.platform.architecture.join("."), settings.platform.compiler, settings.platform.frontendVersion, hashstr); }

And calling this from both places?

Furthermore, you would want to hash lbuildsettings.sourceFiles - not their length.

changelog/cache-generated-test-config.dd

foerdi · 2020-08-29T16:19:17Z

Should I click on Resolve conversation after I pushed changes to the topic or the reviewer resolve the conversation after he is satisfied?

wilzbach · 2020-08-29T16:26:40Z

Should I click on Resolve conversation after I pushed changes to the topic or the reviewer resolve the conversation after he is satisfied?

Yes, please feel free to if it was a simple fix. Maybe be a bit more careful with a more in-depth discussion that wasn't fully resolved or might be relevant to other contributors so that they don't point out the same thing.

foerdi · 2020-08-29T17:51:34Z

Did you have a idea why the osx test runner are failing?

[ERROR] /Users/runner/work/dub/dub/test/cache-generated-test-config.sh:8 command failed
[ERROR] /Users/runner/work/dub/dub/test/cache-generated-test-config.sh:8 command failed
[ERROR] Script failure.

test/cache-generated-test-config.sh:8:

EXECUTABLE_TIME="$(stat cache-generated-test-config-test-library | grep Modify)"

wilzbach · 2020-08-29T17:54:40Z

Did you have a idea why the osx test runner are failing?

[ERROR] /Users/runner/work/dub/dub/test/cache-generated-test-config.sh:8 command failed
[ERROR] /Users/runner/work/dub/dub/test/cache-generated-test-config.sh:8 command failed
[ERROR] Script failure.

test/cache-generated-test-config.sh:8:

EXECUTABLE_TIME="$(stat cache-generated-test-config-test-library | grep Modify)"

No, sorry but I recommend printing the output of stat on failure, s.t. you know what goes on.

foerdi · 2020-08-31T11:57:54Z

Now I have reduced and simplified my code. Theoretically, the time check of changed code files for the main file generator is not necessary due to the module name hashes. So I removed the time check.

I noticed that a single macOS test run failed, but different from the last time, and it looks like this is not related to my changes (?). Can i do something about it?

these changes prevent dub from rebuilding a project every time it runs a unittest.

foerdi · 2020-09-28T11:54:43Z

It would be nice if someone could take the time to review my MR.
Many Thanks

Geod24 · 2021-01-04T01:00:41Z

This seems to have introduced a regression, as the Alpine Linux packages now fail to bootstrap:

foerdi · 2021-01-04T16:02:54Z

This seems to have introduced a regression, as the Alpine Linux packages now fail to bootstrap:

I need some more Information:

Which package is failing to build or test?
How I can reproduce this error?

Maybe, can you check which files are in .dub/code/*?

Geod24 · 2021-01-04T16:59:39Z

I didn't reduce the error yet, as there are other failures. Note that we're building with gdc, so it might be a GDC-specific issue cropping up. The MR is here if you want to take a look. Otherwise I'll get back to it ASAP (so probably this week).

foerdi · 2021-02-02T11:17:06Z

Sorry for my late reply, I forgot to reply about my hollidaies. If this is still a problem, it would be great if you could reduce this error a bit more then I could help you. I couldn't figure out which project couldn't be tested.

Geod24 · 2021-02-22T11:06:18Z

@foerdi : It's the check step of dub itself. Just running bin/dub test --compiler=ldc2 -v after building dub will show this.

Geod24 · 2021-02-22T12:05:30Z

@foerdi : I don't know exactly how to reproduce this, as it's only triggered during the package build process.
What I see is that 2 values are created, one contains the $DFLAGS, and the other nothing, so this also changes the hash.
I solved it be reverting this PR in Alpine Linux.

Geod24 · 2021-02-26T09:40:43Z

@foerdi : I just reproduced this accidentally. Seems to trigger with DFLAGS builds.
E.g. take https://github.com/Geod24/bitblob/ which is a very simple library.
I tried to build it with:

$ DFLAGS='-wi' dub test -v
Using dub registry url 'https://code.dlang.org/'
Refreshing local packages (refresh existing: true)...
Looking for local package map at /var/lib/dub/packages/local-packages.json
Looking for local package map at /Users/geod24/.dub/packages/local-packages.json
Try to load local package map at /Users/geod24/.dub/packages/local-packages.json
Note: Failed to determine version of package custom-tool at .. Assuming ~master.
Looking for local package map at /Users/geod24/projects/geod24/bitblob/.dub/packages/local-packages.json
Determined package version using GIT: bitblob 1.1.4+commit.5.g73eaab4
Refreshing local packages (refresh existing: false)...
Looking for local package map at /var/lib/dub/packages/local-packages.json
Looking for local package map at /Users/geod24/.dub/packages/local-packages.json
Try to load local package map at /Users/geod24/.dub/packages/local-packages.json
Looking for local package map at /Users/geod24/projects/geod24/bitblob/.dub/packages/local-packages.json
Refreshing local packages (refresh existing: false)...
Looking for local package map at /var/lib/dub/packages/local-packages.json
Looking for local package map at /Users/geod24/.dub/packages/local-packages.json
Try to load local package map at /Users/geod24/.dub/packages/local-packages.json
Looking for local package map at /Users/geod24/projects/geod24/bitblob/.dub/packages/local-packages.json
Generating test runner configuration 'bitblob-test-library' for 'library' (library).
Get module name from path: /Users/geod24/projects/geod24/bitblob/source/geod24/bitblob.d
Refreshing local packages (refresh existing: false)...
Looking for local package map at /var/lib/dub/packages/local-packages.json
Looking for local package map at /Users/geod24/.dub/packages/local-packages.json
Try to load local package map at /Users/geod24/.dub/packages/local-packages.json
Looking for local package map at /Users/geod24/projects/geod24/bitblob/.dub/packages/local-packages.json
Configuring dependent bitblob, deps:
Performing "$DFLAGS" build using dmd for x86_64.
Target '/Users/geod24/projects/geod24/bitblob/.dub/build/bitblob-test-library-$DFLAGS-posix.osx.darwin-x86_64-dmd_v2.095.0-18ADA2291157B839D639A99D4340E461/bitblob-test-library' doesn't exist, need rebuild.
bitblob 1.1.4+commit.5.g73eaab4: building configuration "bitblob-test-library"...
dmd -c -of.dub/build/bitblob-test-library-$DFLAGS-posix.osx.darwin-x86_64-dmd_v2.095.0-18ADA2291157B839D639A99D4340E461/bitblob-test-library.o -wi -w -version=Have_bitblob -Isource/ .dub/code/bitblob-test-library--posix.osx.darwin-x86_64-dmd_v2.095.0-BB10D44F4B259993F6B84BBEB68A1E0C_dub_test_root.d source/geod24/bitblob.d -vcolumns
Error: module bitblob-test-library--posix.osx.darwin-x86_64-dmd_v2.095.0-BB10D44F4B259993F6B84BBEB68A1E0C_dub_test_root is in file '.dub/code/bitblob-test-library--posix.osx.darwin-x86_64-dmd_v2.095.0-BB10D44F4B259993F6B84BBEB68A1E0C_dub_test_root.d' which cannot be read
import path[0] = source/
import path[1] = /usr/local/opt/dmd/include/dlang/dmd
FAIL .dub/build/bitblob-test-library-$DFLAGS-posix.osx.darwin-x86_64-dmd_v2.095.0-18ADA2291157B839D639A99D4340E461/ bitblob-test-library executable
dmd failed with exit code 1.
$ ll .dub/code
total 8
-rw-r--r--  1 geod24  staff  555 Feb 26 18:36 bitblob-test-library-$DFLAGS-posix.osx.darwin-x86_64-dmd_v2.095.0-BB10D44F4B259993F6B84BBEB68A1E0C_dub_test_root.d

foerdi · 2021-02-26T11:28:24Z

@Geod24 : Thank you, for your work and the reduced Test Case. Hopefully, I find some time this weekend to fix this bug.
I have already an idea why this could be happening.

Now, how is the correct workflow?
Should I file a bug and a linked pull request, is a simple pull-request enough or can I recycle this pull-request / branch?

foerdi · 2021-03-07T15:38:16Z

@Geod24 Today I found some time and fixed your issue: #2116. Thank you for the discovery.

foerdi force-pushed the cache-generated-test-config branch from 6f363e5 to ce9c64c Compare August 29, 2020 11:20

wilzbach reviewed Aug 29, 2020

View reviewed changes

foerdi force-pushed the cache-generated-test-config branch from ce9c64c to 2d839c2 Compare August 29, 2020 14:57

wilzbach reviewed Aug 29, 2020

View reviewed changes

test/cache-generated-test-config/test.log Outdated Show resolved Hide resolved

foerdi force-pushed the cache-generated-test-config branch from 2d839c2 to b77edc0 Compare August 29, 2020 15:41

wilzbach reviewed Aug 29, 2020

View reviewed changes

foerdi force-pushed the cache-generated-test-config branch from b77edc0 to f78acd9 Compare August 29, 2020 17:27

foerdi force-pushed the cache-generated-test-config branch 9 times, most recently from f743817 to 2b5a180 Compare August 31, 2020 10:56

foerdi force-pushed the cache-generated-test-config branch from 2b5a180 to c86ae4a Compare September 8, 2020 09:56

cache the generated test main file: dub_test_root.d

b600afb

these changes prevent dub from rebuilding a project every time it runs a unittest.

foerdi force-pushed the cache-generated-test-config branch from c86ae4a to b600afb Compare September 28, 2020 09:32

thewilsonator approved these changes Sep 30, 2020

View reviewed changes

thewilsonator added the auto-merge label Sep 30, 2020

dlang-bot merged commit df67d83 into dlang:master Sep 30, 2020

foerdi mentioned this pull request Mar 7, 2021

Fixed a regression with DFLAGS environment variable and cached generated unittest #2116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cache the generated test main file: dub_test_root.d #2005

cache the generated test main file: dub_test_root.d #2005

foerdi commented Aug 29, 2020

dlang-bot commented Aug 29, 2020

wilzbach left a comment

wilzbach Aug 29, 2020

foerdi Aug 29, 2020

wilzbach Aug 29, 2020

foerdi Aug 29, 2020

wilzbach Aug 29, 2020

foerdi commented Aug 29, 2020

wilzbach commented Aug 29, 2020

foerdi commented Aug 29, 2020 •

edited

Loading

wilzbach commented Aug 29, 2020

foerdi commented Aug 31, 2020

foerdi commented Sep 28, 2020

Geod24 commented Jan 4, 2021

foerdi commented Jan 4, 2021

Geod24 commented Jan 4, 2021

foerdi commented Feb 2, 2021

Geod24 commented Feb 22, 2021

Geod24 commented Feb 22, 2021

Geod24 commented Feb 26, 2021

foerdi commented Feb 26, 2021

foerdi commented Mar 7, 2021

cache the generated test main file: dub_test_root.d #2005

cache the generated test main file: dub_test_root.d #2005

Conversation

foerdi commented Aug 29, 2020

dlang-bot commented Aug 29, 2020

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach Aug 29, 2020

Choose a reason for hiding this comment

foerdi Aug 29, 2020

Choose a reason for hiding this comment

wilzbach Aug 29, 2020

Choose a reason for hiding this comment

foerdi Aug 29, 2020

Choose a reason for hiding this comment

wilzbach Aug 29, 2020

Choose a reason for hiding this comment

foerdi commented Aug 29, 2020

wilzbach commented Aug 29, 2020

foerdi commented Aug 29, 2020 • edited Loading

wilzbach commented Aug 29, 2020

foerdi commented Aug 31, 2020

foerdi commented Sep 28, 2020

Geod24 commented Jan 4, 2021

foerdi commented Jan 4, 2021

Geod24 commented Jan 4, 2021

foerdi commented Feb 2, 2021

Geod24 commented Feb 22, 2021

Geod24 commented Feb 22, 2021

Geod24 commented Feb 26, 2021

foerdi commented Feb 26, 2021

foerdi commented Mar 7, 2021

foerdi commented Aug 29, 2020 •

edited

Loading