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

Make the current artifact configurable and default to use the jar #143

Merged
merged 5 commits into from
Nov 14, 2023

Conversation

lefou
Copy link
Collaborator

@lefou lefou commented Nov 14, 2023

This enhances the flexibility of the plugin. It defaults to jar to also take all potential post processing (filtering, shading, OSGi bundling) into account.
This is IMHO a better default then compile().classes, but if someone needs the old behavior, it's now configurable.

@lefou
Copy link
Collaborator Author

lefou commented Nov 14, 2023

This PR is my preferred alternative to #142

@lolgab
Copy link
Owner

lolgab commented Nov 14, 2023

Thank you for the contribution.
I would make it configurable but I wouldn't change the default for two reasons.

  1. the current default matches the official plugin for sbt. It's less likely to see surprises when Sbt users migrate to Mill if we use the same defaults, when possible.
  2. Avoiding building the jar can give a faster loop while fixing issues.

We might want to document the setting to allow power users of mill-mima to use the jar when needed.

@lefou
Copy link
Collaborator Author

lefou commented Nov 14, 2023

It's your plugin, so your choice, but let me further motivate. My main argument is: It's the only correct behavior! The sbt plugin is probably less correct. It makes no sense at all to check not against the jar, except when it's less performant and the assumption is, that both result in the same outcome (same classpath). Creating the jar isn't free, but checking binary compatibility against other jars which need to be downloaded isn't free either. I'd assume, the use case for Mima should not trait performance over correctness. In the end, it's the main goal of Mima to ensure correctly releasable binary-compatible jars. So we should check the jars. (Mill isn't sbt, we always favor correctness over convenience. Take the scoverage for example, where in sbt you need to clear the previous compilation and override the classes with scoverage-enabled bytecode, whereas in Mill, we have a dedicated sub-module for that, so you can never accidentally end up with the wrong classfiles. Mill plugins should embrace the same thinking.)

I think, there is just no one who already encountered the issues, which I recently faced, and reported it against sbt-mima. Probably because sbt isn't used for that kind of customized or polyglot builds. I encountered this issue in coursier, which does some classpath shading in the artifacts, and comparing the pure compile outcome to compare against older released jars is always wrong.

But I also work with other setups which have polyglot compiler toolchains producing jars in the end. If I would apply Mima to those project, using the jar would always be correct, but using compile wouldn't.

This enhances the flexibility of the plugin.
This is to also take all potential post processing (filtering, shading, OSGi bundling) into account.
This is IMHO a better default then `compile().classes`, but if someone needs to old behavior, it's now configurable.
@lolgab
Copy link
Owner

lolgab commented Nov 14, 2023

Your reasoning makes sense. Thank you for explaining.
I hope that Sbt makes the same choice, so we can also have consistency across Scala tools.

@lolgab
Copy link
Owner

lolgab commented Nov 14, 2023

Can you add the new mimaCurrentArtifact configuration to the documentation as well?

@lefou
Copy link
Collaborator Author

lefou commented Nov 14, 2023

Your reasoning makes sense. Thank you for explaining. I hope that Sbt makes the same choice, so we can also have consistency across Scala tools.

I agree. I would open that pull request to sbt-mima myself, but I'd rather not do so, as I have no current project to confidentially test it. Also, I lack confident up-to-date knowledge of sbt and do not 100% understand the changes I need to do. (I once PR'ed to sbt-osgi plugin which resulted in a race condition with a later sbt release. It's now settled, but I'm aware that it is not enough to get something working in sbt. You must deeply know how target use each other, as they can modify each others content. depending on the dependency graph alone is not enough.)

Can you add the new mimaCurrentArtifact configuration to the documentation as well?

Yes.

@lolgab lolgab merged commit 8ca13c3 into lolgab:main Nov 14, 2023
2 of 3 checks passed
@lefou lefou deleted the use-jar-2 branch November 14, 2023 14:01
@lefou
Copy link
Collaborator Author

lefou commented Nov 14, 2023

Thanks for merging! Could you cut a new release, please?

@lolgab
Copy link
Owner

lolgab commented Nov 14, 2023

What tag would you suggest? 0.0.25 or 0.1.0?

@lefou
Copy link
Collaborator Author

lefou commented Nov 14, 2023

Both are ok. 0.1.0 sound more mature. ;)

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.

2 participants