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.

Sign up for GitHub

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

Jump to bottom

Detect native source file relocation during recompilation #29557

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

Detect native source file relocation during recompilation #29557

wants to merge 6 commits into from
55 −202

Conversation

lacasseio
Copy link
Contributor

Fixes #29492

Context

Contributor Checklist

  • Review Contribution Guidelines.
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team.
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective.
  • [N/A] Provide unit tests (under <subproject>/src/test) to verify logic.
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes.
  • Ensure that tests pass sanity check: ./gradlew sanityCheck.
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest.

Reviewing cheatsheet

Before merging the PR, comments starting with

  • ❌ ❓must be fixed
  • 🤔 💅 should be fixed
  • 💭 may be fixed
  • 🎉 celebrate happy things

@lacasseio lacasseio requested review from a team as code owners June 15, 2024 11:07
@bot-gradle bot-gradle added from:contributor PR by an external contributor to-triage labels Jun 15, 2024
@big-guy big-guy self-assigned this Jun 17, 2024
@big-guy big-guy self-requested a review June 17, 2024 15:19
@big-guy big-guy added the 👋 team-triage Issues that need to be triaged by a specific team label Jun 17, 2024
@big-guy big-guy added this to the 8.10 RC1 milestone Jun 17, 2024
@ljacomet ljacomet added in:native-platform c, cpp, swift and other native languages support, etc and removed to-triage labels Jun 17, 2024
@big-guy big-guy removed the 👋 team-triage Issues that need to be triaged by a specific team label Jun 17, 2024
@gradle gradle deleted a comment from big-guy Jun 17, 2024
@bot-gradle bot-gradle added this pull request to the merge queue Jun 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 17, 2024
@big-guy
Finalize source file collection
897f0d4 
See if this impacts performance measurements
@big-guy
Copy link
Member

big-guy commented Jun 21, 2024

@bot-gradle rfn

@gradle gradle deleted a comment from big-guy Jun 21, 2024
@bot-gradle
Copy link
Collaborator

Sorry I don't understand what you said.
Currently, the following commands are supported:

@bot-gradle test <BuildTrigger1> <BuildTrigger2> ... <BuildTriggerN> [without PTS]

Add without PTS to run the build with full test coverage.

  • A trigger is a special build for this PR on TeamCity, common triggers are:
    • SanityCheck/CompileAll/QuickFeedbackLinux/QuickFeedback/PullRequestFeedback/ReadyForNightly/ReadyForRelease
    • Shortcuts: SC/CA/QFL/QF/PRF/RFN/RFR
    • Specific builds:
      • BD: BuildDistributions/BuildDocs so that you can preview the generated docs/distribution.
      • PT: PerformanceTest, all performance tests for Ready For Nightly stage.
      • APT: AllPerformanceTest, all performance tests, including slow performance tests.
      • AST: AllSmokeTestsPullRequestFeedback
      • AFT: AllFunctionalTestsPullRequestFeedback
      • ASB: AllSpecificBuildsPullRequestFeedback
      • ACC: AllConfigCacheTestsPullRequestFeedback
      • ACT: AllCrossVersionTestsReadyForNightly
      • AFTN: AllFunctionalTestsReadyForNightly
      • ACTR: AllCrossVersionTestsReadyForRelease
      • AFTR: AllFunctionalTestsReadyForRelease

@bot-gradle squash

  • Squash the current pull request into a single commit. The squash message will come from the pull request body, with:
    • All Signed-off-by: lines kept.
    • All authors being Co-Authored-By:

@bot-gradle merge

  • Enqueue this PR into GitHub merge queue for merging.
    • GitHub will create a merge commit from your PR branch HEAD and the target branch
    • A GitHub Merge Queue Check Pass build will be triggered on the merge commit
    • When the build passes, the target branch will be fast-forwarded to this merge commit (i.e. merge the PR)

@bot-gradle squash and merge

  • Squash the current pull request into a single commit as described in squash command above,
    then enqueue this PR into GitHub merge queue as described in merge command above.

@bot-gradle cherrypick to <branch>

  • Cherrypicks the current PR to another branch.
    A new PR will be created and a build will triggered automatically if there is no conflict.

@bot-gradle cancel

  • cancel a running build in GitHub merge queue and remove the PR from the queue

@bot-gradle clean

  • clear the conversation history

@bot-gradle help

  • display this message

To run a command, simply submit a comment. For detailed instructions see here.

@big-guy
Copy link
Member

big-guy commented Jun 21, 2024

@bot-gradle test RFN

@gradle gradle deleted a comment from big-guy Jun 21, 2024
@bot-gradle
Copy link
Collaborator

I've triggered the following builds for you. Click here to see all build failures.

lacasseio
lacasseio commented Jun 28, 2024
View reviewed changes
...native/src/main/java/org/gradle/language/nativeplatform/tasks/AbstractNativeCompileTask.java
this.targetPlatform = objectFactory.property(NativePlatform.class);
this.toolChain = objectFactory.property(NativeToolChain.class);
this.source = getObjects().fileCollection();
this.source.finalizeValueOnRead();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

❌ @big-guy Unless the bug with finalizeValueOnRead() where it discards task dependencies when read was fixed, this can cause issues. I don't think I had a chance to open the issue (or it was in our shared document), but when the file collection is realized, the task dependencies are dropped. I don't remember if it was a bug or intended behaviour. It would be worth checking that is someone early reads the source, everything works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some code on v7.6.4 that will exhibit the explained behaviour above. If it was fixed on 8.x, backporting this fix to 7.6.x will need to be adjusted.

@big-guy big-guy modified the milestones: 8.10 RC1, 8.11 RC1 Jul 23, 2024
@big-guy big-guy removed this from the 8.11 RC1 milestone Aug 6, 2024
@big-guy big-guy added this to the 8.12 RC1 milestone Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@big-guy big-guy Awaiting requested review from big-guy

Assignees

@big-guy big-guy

Labels
from:contributor PR by an external contributor in:native-platform c, cpp, swift and other native languages support, etc
Projects
None yet
Milestone
8.12 RC1
Development

Successfully merging this pull request may close these issues.

CachingTaskInputFileCollection loose relative path sensitivity
4 participants
@lacasseio @big-guy @bot-gradle @ljacomet