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

Bump uPickle to 2.0.0 together with Ammonite 2.5.4 #1866

Merged
merged 2 commits into from
May 31, 2022
Merged

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented May 6, 2022

Second attempt at landing #1854. Let's see if keeping the uPickle version consistent between Ammonite and Mill helps

I tried to repro the original problem using the locally-built target/mill-release binary, and am able to reproduce the problem (unwanted re-compilation of build.sc) without the Ammonite update, and am unable to reproduce the problem with the Ammonite update. I don't know what the problem is, but I can only conclude that the Ammonite update fixed it.

@lolgab could you help try this out see if it works properly for you too?

@lolgab
Copy link
Member

lolgab commented May 6, 2022

Yes, I can try!
Still, I think it is a good idea to release Mill 0.10.4 before merging this, so we can safely release the bug fixes we have in master without risking any regression.

@lefou
Copy link
Member

lefou commented May 6, 2022

Yes, I can try! Still, I think it is a good idea to release Mill 0.10.4 before merging this, so we can safely release the bug fixes we have in master without risking any regression.

Mill 0.10.4 is already released.

@lolgab
Copy link
Member

lolgab commented May 7, 2022

@lihaoyi Yes updating uPickle and ammonite solves the problem for me as well.
To double-check I tried to downgrade ammonite only the problem appeared again.

@lefou
Copy link
Member

lefou commented May 7, 2022

Nice. I'd like to wait a little bit longer so we can use a proper Ammonite release, though. In the past, snapshot releases of Ammonite made it hard for me to reason about the changes and the steps of action needed to update.

@lolgab
Copy link
Member

lolgab commented May 21, 2022

We need to be sure that plugins compiled with Mill 0.10.0 still work as expected after this change.

@lihaoyi
Copy link
Member Author

lihaoyi commented May 21, 2022

@lolgab I believe they should.

This is a format change, but it is backwards compatible and should not break, and regardless Mill doesn't care about compatibility of the format because we flush all caches when classpath changes.

w.r.t. Binary compatibility, I believe it should be compatible unless you're writing custom Visitors. I assume most Mill plugins (and users!) use it as a black box and do not work with the underlying Visitor API directly. So I think it should be safe

@lolgab
Copy link
Member

lolgab commented May 21, 2022

@lihaoyi I also believe this should work just fine, but let's manual-test some plugins to check if we don't get weird cache invalidations as we got with the build when there were two different versions of uPickle in the build.

@lihaoyi
Copy link
Member Author

lihaoyi commented May 21, 2022

@lolgab I don't have any plugins myself, if you have some in mind feel free to test them :)

@lefou lefou changed the title Bump uPickle to 2.0.0 together with Ammonite Bump uPickle to 2.0.0 together with Ammonite 2.5.4 May 21, 2022
@lihaoyi
Copy link
Member Author

lihaoyi commented May 31, 2022

@lolgab any updates here? If not I feel like we can merge this as is.

Copy link
Member

@lolgab lolgab left a comment

Choose a reason for hiding this comment

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

@lihaoyi Sorry for the big late. I tried it on one project of mine and updating mill without updating any plugins doesn't create any cache invalidation problems.
LGTM! 👍

@lefou lefou merged commit 1a2fbde into main May 31, 2022
@lefou lefou deleted the bump-upickle-2 branch May 31, 2022 11:35
@lefou lefou added this to the after 0.10.4 milestone May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants