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

Omit the suffix in artifactName for cross modules #953

Merged
merged 1 commit into from
Oct 21, 2020

Conversation

joan38
Copy link
Collaborator

@joan38 joan38 commented Aug 31, 2020

Users of mill (including me) struggle with publishing artifacts as soon as they start cross compiling between multiple Scala versions.
By default it will publish Artifact(org,myproject-2.12.10_2.12,0.0.1-SNAPSHOT) where we expect something like Artifact(org,myproject_2.12,0.0.1-SNAPSHOT).
Here is an example: https://gitter.im/lihaoyi/mill?at=5f404a975580fa092b15a821

The usual fix is to override the artifactName as follows:

def artifactName = "myproject"

I personally get caught every time I start to cross compile a project and believe that doesn't look reasonable to ask the user to course correct the artifact name.

This proposal removes the added segment part in artifactName for CrossModuleBase.

cc @davesmith00000

@davesmith00000
Copy link
Contributor

I have no idea if that will work, but I'm excited! Thanks for looking into this @joan38. :-)

@joan38
Copy link
Collaborator Author

joan38 commented Oct 3, 2020

@lefou @lihaoyi Any thoughts?

@lefou
Copy link
Member

lefou commented Oct 5, 2020

I'd like to see a test case covering (and documenting) the expected outcome.

@lihaoyi
Copy link
Member

lihaoyi commented Oct 5, 2020

I'm a bit reluctant to do this, due to silently changing the existing behavior. OTOH if the existing behavior is confusing enough, then perhaps it's worth breaking. @joan38 perhaps you could flesh out the description a bit so someone looking at this PR can understand the issue without any external context?

@joan38
Copy link
Collaborator Author

joan38 commented Oct 21, 2020

@lihaoyi @lefou both done

@lihaoyi
Copy link
Member

lihaoyi commented Oct 21, 2020

I think this looks good to me

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I just wanted to mention, that at first I struggled with the idea, because in cross-compiling scenarios outside of Scala modules the current behaviour (appending the cross-parameter to the artifact name) might be more appropriate. Later I realized, that CrossModuleBase is only used for CrossScalaModule and CrossSbtModule, so yes, this PR makes lots of sense, as it should not affect other modules created with Cross[T].

@lefou lefou merged commit 81ae1dc into com-lihaoyi:master Oct 21, 2020
@lefou lefou added this to the after 0.8.0 milestone Oct 21, 2020
@lefou
Copy link
Member

lefou commented Oct 21, 2020

Thanks, @joan38 !

@joan38 joan38 deleted the cross-artifactName branch October 21, 2020 15:57
@joan38
Copy link
Collaborator Author

joan38 commented Oct 22, 2020

Make sense, thanks!

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.

None yet

4 participants