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

Sync select files #7127

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

odisseus
Copy link
Contributor

@odisseus odisseus commented Dec 6, 2024

Checklist

  • I have filed an issue about this change and discussed potential changes with the maintainers.
  • I have received the approval from the maintainers to make this change.
  • This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: <please reference the issue number or url here>

Description of this change

Copy link

google-cla bot commented Dec 6, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@odisseus odisseus force-pushed the sync-select-files branch 4 times, most recently from dbcd958 to 155c4c2 Compare December 10, 2024 05:40
@odisseus odisseus marked this pull request as ready for review December 10, 2024 05:42
@github-actions github-actions bot added product: CLion CLion plugin product: IntelliJ IntelliJ plugin product: GoLand GoLand plugin awaiting-review Awaiting review from Bazel team on PRs labels Dec 10, 2024
@@ -183,7 +193,7 @@ public void afterQuerySync(Project project, BlazeContext context) {
return;
}
if (requester.changePending.get()) {
requester.requestSyncInternal();
requester.requestSyncInternal(ImmutableList.of());
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work. Consider a sequence

  1. File 1 is added
  2. Sync Starts
  3. File 2 is added
  4. Sync Finishes
  5. afterQuerySync is called (this line)

File 2 won't be synced. The afterQuerySync is supposed to be executed here nad catch File 2

@tpasternak
Copy link
Contributor

how about something lile that? #7168

@tpasternak
Copy link
Contributor

Unfortunately I just noticed that the refresh logic is deeply tight with vcs ( Bazel -> Expand Sync To Working Set) and it is not enough to just use QuerySyncActionStatsScope.createForFiles

The problem is here, the requiresFullUpdate and calculateAffectedPackages ignore what is set in createForFiles and they fall back to git-based delta sync or full sync.

if (params.requiresFullUpdate(context)) {
return startFullUpdate(
context,
params.latestProjectDefinition,
params.latestVcsState,
params.latestBazelVersion);
}
AffectedPackages affected = params.calculateAffectedPackages(context);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review Awaiting review from Bazel team on PRs product: CLion CLion plugin product: GoLand GoLand plugin product: IntelliJ IntelliJ plugin
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

5 participants