-
-
Notifications
You must be signed in to change notification settings - Fork 230
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 unnecessary rebuild of single file package #1811
Conversation
Thanks for your pull request and interest in making D better, @andre2007! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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. |
version.d was added by mistake, i will remove it from pr |
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.
A minor nit, but lgtm afaics
Ping:) semaphoreci & appveyor issues are unrelated. |
Reopening pr to trigger voters again |
The only thing I think should be improved now is that the variable is still called "saveSelections". I think some name that would better represent the documentation and the case what it actually does (rebuild if dub.selections.json is modified) would be more appropriate. Makes it easier to construct and read |
@WebFreak001 rebuilding is a decision at some specific point in the program flow based on the information that the selections are saved. Therefore I am not sure about changing the variable name, because.. But if you like I change the variable name, do you have a proposal? |
hm you are right but the variable isn't used for anything else and I don't really see any other uses in it in the future. Maybe this shouldn't be stored in GeneratorSettings at all like this, but rather only store "single" in generator settings (because generators might be interested in single more) and then determine
|
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.
Looks good to me, nice work!
@andre2007 The lastest revision of your PR looks like a pretty clean change, P.S. Thanks for following through on the problem and taking it to the finish line. I meant to work on this myself, but I haven't had enough free time lately. Edit: looks like it's working now... |
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.
LGTM
With my current release dub I don't seem to be getting the "To force a rebuild" message that would make the test pass here, but I think this might just be something with my old dub version I have (and I can't build the new one because registry has a major downtime right now)
reopen to force a restart of pr voters |
@WebFreak001 @PetarKirov Unfortunately the pr voters are all broken. See https://forum.dlang.org/post/iqbsiwwzcqylnzznbemx@forum.dlang.org |
In case a single file package has dependencies, isUpToDate method checks whether
dub.selections.json
exists. As single file package has the UpgradeOptions.noSaveSelectionsdub.selections.json
won't be created and therefore isUpToDate always returns false.This pull request passes the UpgradeOptions.noSaveSelections to the build generator and avoid the check of
dub.selections.json