Skip to content

Comments

build.d: Save whether dependency was updated#10643

Merged
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:saveUpdated
Dec 12, 2019
Merged

build.d: Save whether dependency was updated#10643
dlang-bot merged 1 commit intodlang:masterfrom
marler8997:saveUpdated

Conversation

@marler8997
Copy link
Contributor

@marler8997 marler8997 commented Dec 7, 2019

Fixes a race condition caused by b3621e8

The problem is line 1420 return false;. The first time something calls run it will return whether or not it was updated correctly, but all future calls will return false. This introduces a race condition on whether or not the caller will see that the dependency was updated. The expected behavior is that the run function will always return true (meaning the dependency was updated in the current build) if it's command was executed, which signals that all the "dependents" will also need to be rebuilt.

Note that I also removed the return type from runSynchronized, which guarantees that it will always return the same updated state. There's no chance that you could have one statement say return true or return false and return an updated state that doesn't match what it stored in the updated field.

@dlang-bot
Copy link
Contributor

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

Bugzilla references

Your 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 locally

If 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#10643"

@marler8997 marler8997 changed the title Save whether dependency was updated build.d: Save whether dependency was updated Dec 7, 2019
@dlang-bot dlang-bot merged commit cbdc9b1 into dlang:master Dec 12, 2019
@marler8997 marler8997 deleted the saveUpdated branch December 12, 2019 21:16
@WalterBright
Copy link
Member

@marler8997 something has gone terribly wrong with building dmd. It used to build fairly quickly. Now I change one file, do a "debug" build, and it takes about 5 minutes. Debug builds are supposed to be fast, and they used to be fast. I'm doing a:

make -f posix.mak ENABLE_RELEASE= ENABLE_DEBUG=1 -j 4

@WalterBright
Copy link
Member

This is killing my productivity.

@MoonlightSentinel
Copy link
Contributor

MoonlightSentinel commented Dec 13, 2019

@marler8997 something has gone terribly wrong with building dmd. It used to build fairly quickly. Now I change one file, do a "debug" build, and it takes about 5 minutes. Debug builds are supposed to be fast, and they used to be fast. I'm doing a:

make -f posix.mak ENABLE_RELEASE= ENABLE_DEBUG=1 -j 4

@WalterBright Does it also take that much time using the following command?

rdmd src/build ENABLE_RELEASE=0 ENABLE_DEBUG=1

EDIT; ENABLE_RELEASE= is currently equivalent to ENABLE_RELEASE=1

@marler8997
Copy link
Contributor Author

marler8997 commented Dec 13, 2019

Looks like the problem is that build.d it interpreting ENABLE_RELEASE= as true. (empty value is being interpreted as true). Putting together a PR to fix...

@marler8997
Copy link
Contributor Author

marler8997 commented Dec 13, 2019

fix here: #10663

note that problems like this are going to happen during this transition. That's why we are making changes gradually so we can isolate and identify issues quickly. However, I think this issue has likely been around for many months so I'm not sure why this is the first time someone has seen/reported it.

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.

5 participants