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

Fixed visibility of atomic properties as required by the new JVM compiler plugin #3808

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Fixed visibility of atomic properties as required by the new JVM compiler plugin #3808

merged 1 commit into from
Jul 18, 2023

Conversation

mvicsokolova
Copy link
Contributor

The new atomicfu JVM compiler plugin (since 1.9.20) will explicitly require atomic properties to be private or internal (or be members of private or internal classes). These fixes are required now in the aggregate build to merge changes in the new jvm plugin, so this commit is added to develop first.

qwwdfsad
qwwdfsad reviewed Jul 17, 2023
View reviewed changes
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Please don't forget to put kotlin-merge-blocker PRs labeled as merge-blockers are effectively preventing a corresponding merge into main Kotlin repo on the PR and mention the corresponding issue (whether it's KT/atomicfu GH or just a PR), so we can keep track of the change later

@mvicsokolova
Copy link
Contributor Author

I'll add the link to this issue at atomicfu GH before merge: Kotlin/kotlinx-atomicfu#322

@mvicsokolova
Copy link
Contributor Author

mvicsokolova commented Jul 18, 2023 •
edited
Loading

Should we put kotlin-merge-blocker label on PRs that are merged into develop?
I thought it was only supposed to be used for PRs to kotlin-community/* branches that can not be merged in develop but necessary for the aggregate 🙄

@mvicsokolova mvicsokolova force-pushed the applying-new-atomicfu-jvm-plugin branch from a425482 to 9f4485e Compare July 18, 2023 08:33
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Jul 18, 2023 •
edited
Loading

Should we put kotlin-merge-blocker label on PRs that are merged into develop?

Yes, on everything that breaks aggregate. This is also a signal for release manager to proceed with rebase, see https://jetbrains.team/p/kti/repositories/kotlin-teamcity-build/files/.teamcity/common/buildtypes/tests/userprojects/mainAggregate/README.md
Also, a reviewer from release team is required

qwwdfsad
qwwdfsad approved these changes Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

LGTM, but please merge only after every step from the aggregate process is fulfilled

@mvicsokolova
Copy link
Contributor Author

Ok, thanks! Though this should not break the aggregate build.
All the changes in the JVM compiler plugin are still in MR (https://jetbrains.team/p/kt/reviews/10579/timeline) and not merged because the aggregate is red and needs these changes in the kotlin-community/* branch.

@mvicsokolova mvicsokolova added the kotlin-merge-blocker PRs labeled as merge-blockers are effectively preventing a corresponding merge into main Kotlin repo label Jul 18, 2023
@mshishkina
Copy link

Requested also KT issue as changes are done in Kotlin master.

@mvicsokolova
Copy link
Contributor Author

mvicsokolova commented Jul 18, 2023 •
edited
Loading

Here is the YT issue for all the changes: https://youtrack.jetbrains.com/issue/KT-60528/Updates-for-JVM-IR-backend-of-kotlin-atomicfu-compiler-plugin

Mentioned it in the commit message

@mvicsokolova
Fixed visibility of atomic properties, they are required to be privat…
eeba1be 
…e or internal

The new atomicfu JVM compiler plugin (since 1.9.20) will explicitly require atomic properties to be private or internal (or to be members of private or internal classes).

See: Kotlin/kotlinx-atomicfu#322
YT issue: KT-60528
@mvicsokolova mvicsokolova force-pushed the applying-new-atomicfu-jvm-plugin branch from 9f4485e to eeba1be Compare July 18, 2023 14:58
@mvicsokolova mvicsokolova merged commit 38909c7 into develop Jul 18, 2023
@mvicsokolova mvicsokolova deleted the applying-new-atomicfu-jvm-plugin branch July 18, 2023 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qwwdfsad qwwdfsad qwwdfsad approved these changes

@mshishkina mshishkina mshishkina approved these changes

Assignees
No one assigned
Labels
kotlin-merge-blocker PRs labeled as merge-blockers are effectively preventing a corresponding merge into main Kotlin repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mvicsokolova @qwwdfsad @mshishkina