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(dub): Make config.d executable to incremental compilation #13712

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

ljmf00
Copy link
Member

@ljmf00 ljmf00 commented Feb 23, 2022

Signed-off-by: Luís Ferreira contact@lsferreira.net


Before:

Running pre-generate commands for dmd:lexer...
parsePackageRecipe dub.sdl
Performing "debug" build using /usr/bin/dmd for x86_64.
config ~master: building configuration "application"...
Linking...
Running generated/dub/config /home/luis/Workspace/Programming/Repos/temp/dmdlint/dmd/generated/dub /home/luis/Workspace/Programming/Repos/temp/dmdlint/dmd/VERSION /etc
Performing "debug" build using /usr/bin/dmd for x86_64.
dmd:root ~master: target for configuration "library" is up to date.
dmd:lexer ~master: building configuration "library"...
dmd:parser ~master: building configuration "library"...
dmd:frontend ~master: building configuration "library"...

After:

Running pre-generate commands for dmd:lexer...
Performing "debug" build using /usr/bin/dmd for x86_64.
dmd:root ~master: target for configuration "library" is up to date.
dmd:lexer ~master: target for configuration "library" is up to date.
dmd:parser ~master: target for configuration "library" is up to date.
dmd:frontend ~master: target for configuration "library" is up to date.

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 23, 2022

Thanks for your pull request, @ljmf00!

Bugzilla references

Auto-close Bugzilla Severity Description
22821 enhancement Dub package does not use incremental compilation

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#13712"

@@ -1,3 +1,4 @@
#!/usr/bin/env dub
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this executable, in case someone wants to run it as a script on POSIX systems.

dub.sdl Outdated
--arch=$${DUB_ARCH} \
--compiler=$${DC} \
--single "$${DUB_PACKAGE_DIR}config.d" \
"$${DUB_PACKAGE_DIR}config.d" \
Copy link
Member Author

@ljmf00 ljmf00 Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, this dub call doesn't incrementally compile, but calling it directly does. This shouldn't be needed if dub works correctly and does make the library itself incrementally compile, but for the binary itself, this is preferred for now as it is more performant on subsequent runs

@ljmf00 ljmf00 marked this pull request as ready for review February 23, 2022 16:07
@ljmf00
Copy link
Member Author

ljmf00 commented Feb 23, 2022

@CyberShadow Is this infrastructure open? It should provide the full absolute compiler path in order to work, instead of relying on paths relative to cwd.

@CyberShadow
Copy link
Member

This can't possibly work on Windows, right? It has no notion of shebangs or executable files.

@CyberShadow Is this infrastructure open?

Yes: https://github.com/cybershadow/ae/blob/master/sys/d/manager.d

It should provide the full absolute compiler path in order to work, instead of relying on paths relative to cwd.

Not sure if it would be fair to impose such a requirement on everyone.

@ljmf00
Copy link
Member Author

ljmf00 commented Feb 23, 2022

This can't possibly work on Windows, right? It has no notion of shebangs or executable files.

It works as the compiler safely ignores the shebang. Also, for Windows, a dub call is used. I'm only changing the way is called for POSIX platforms, but see bellow. Anyway, dub should fix those issues but I'm not willing to fix them due to how messy dub is.

@CyberShadow Is this infrastructure open?

Yes: cybershadow/ae@master/sys/d/manager.d

It should provide the full absolute compiler path in order to work, instead of relying on paths relative to cwd.

Not sure if it would be fair to impose such a requirement on everyone.

Right, that is a problem, and probably dub shouldn't produce this behaviour, but given that rationale, I will revert this.

@CyberShadow
Copy link
Member

It works as the compiler safely ignores the shebang. Also, for Windows, a dub call is used.

Ah yes, I see the platform="posix" now, thanks for the clarification. 👍

Fix issue 22821.

Signed-off-by: Luís Ferreira <contact@lsferreira.net>
@RazvanN7
Copy link
Contributor

How does this make config.d do incremental compilation?

@ljmf00
Copy link
Member Author

ljmf00 commented Feb 28, 2022

How does this make config.d do incremental compilation?

The generated folder for config binary should be in a different folder, other than the import paths. If it is the same, dub detect changes and recompile it.

@RazvanN7 RazvanN7 merged commit cf63dd8 into dlang:master Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants