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

Cross build with sbt 1.0 #1000

Merged
merged 47 commits into from
Aug 10, 2017
Merged

Cross build with sbt 1.0 #1000

merged 47 commits into from
Aug 10, 2017

Conversation

muuki88
Copy link
Contributor

@muuki88 muuki88 commented Jun 19, 2017

Initial attempt to cross build sbt-native-packager for sbt 1.0.0-M6 and 0.13.15.

Inspired by and copied from

@muuki88 muuki88 requested review from eed3si9n and dwijnand June 19, 2017 19:18
Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Thanks for porting this important plugin.
Let us know if you found some awkward parts in the API.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

I echo Eugene: let us know if any parts of the API or the migration process was awkward/difficult/complicated so we can try and smooth the edges before the release. Thanks.

@muuki88
Copy link
Contributor Author

muuki88 commented Jun 24, 2017

First of all: Even this is the first major version bump of SBT and native-packager is a pretty old plugin, the necessary changes are really small and comprehensible.

I mostly struggled with the the PathFinder changes as some operators are gone, the dependency has to be explicitly added and I had to figure out were the new implicit conversions are located :D But nothing awkward or confusing.

What puzzles me at the moment is the changed behavior with special artifacts and artifact scopes. We have two provided dependencies:

// used in the DockerSpotifyClientPlugin
"com.spotify" % "docker-client" % "3.5.13" % "provided" 

// used in the JDebPackaging plugin
"org.vafer" % "jdeb" % "1.3" % "provided" artifacts Artifact("jdeb", "jar", "jar")

The provided scope used to work for all scripted tests, which means

  • I need to add the dependency explicitly if I want to test the plugin
  • If the plugin is not being activated, then the classes aren't loaded

Apparently this lazy loading behavior changed as I now get ClassNotFound exceptions in every scripted test. Removing the provided scope fixed the issue.

The second thing that puzzled me was that the jdeb dependency still wasn't working with ClassNotFound exceptions. "org.vafer" % "jdeb" % "1.3" is a maven plugin, thus a special artifact is added ( as described in sbt/sbt#499 ). However this doesn't seem to be sufficient enough. I applied the same fix we have in place for coursier ( 2e5b9ec ) and added

classpathTypes += "maven-plugin"

to the native-packager build.sbt. I think this change makes totally sense, but I guess this could be a problem for some other plugin authors.

@sbt sbt deleted a comment Jun 24, 2017
.settings(mySettings)
.settings(
ivyConfigurations += assets,
artifact in assets := artifact.value.withClassifier(classifier = Some("assets")),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eed3si9n Where can I find the latest Artifact API definition? I wasn't able to find the publish API documentation, or the sources files in the sbt-librarymanagment project. I actually guessed this method call 😛
Before it was copy(classifier = Some("assets"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sbt docs on Aritfacts still state there is a copy method

Copy link
Member

Choose a reason for hiding this comment

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

It's generated from https://github.com/sbt/librarymanagement/blob/1.0/librarymanagement/src/main/contraband/librarymanagement.json. I should make sure the generated code goes into GitHub like other sbt modules.

@sbt sbt deleted a comment Jul 1, 2017
@sbt sbt deleted a comment Jul 9, 2017
@sbt sbt deleted a comment Jul 10, 2017
@sbt sbt deleted a comment Jul 12, 2017
@sbt sbt deleted a comment Jul 12, 2017
@muuki88 muuki88 force-pushed the wip/cross-build-sbt-1.0 branch from a0f386e to dbf196f Compare July 17, 2017 17:55
@muuki88 muuki88 force-pushed the wip/cross-build-sbt-1.0 branch from 37866fd to 88233b3 Compare August 7, 2017 17:39
@bpholt
Copy link

bpholt commented Aug 7, 2017

Is there anything else that needs to be done to get this merged? I'm working on cross-building my company's plugins for sbt 1.0 and like @eed3si9n said, this is an important one that's included in several of ours as a dependency. If there's something I can help with, please let me know.

@muuki88
Copy link
Contributor Author

muuki88 commented Aug 8, 2017

Thanks a lot for your offer 😍
Are you using one of the "Deployment" plugins like UniversalDeploymentPlugin? If so, could you test it with this branch?

I tried to release a first RC1 with this, but messed up at some point. The release now takes almost 1 1/2 hours ony machine due to the scripted tests in two versions.

@muuki88
Copy link
Contributor Author

muuki88 commented Aug 8, 2017

The first release candidate is out 1.2.2-RC2

@bpholt
Copy link

bpholt commented Aug 8, 2017

@muuki88 We are using DockerPlugin with JavaServerAppPackaging and as far as I can tell, it's working fine. I built and published it locally, and pulled it into our sbt-docker-containers plugin as well as the private plugin we maintain with default settings for our apps, and built and ran an app locally.

I'll test again with 1.2.2-RC2 but I think we're on the right track! 😃

I did notice a few eviction warnings during the build, which may or may not be a problem. We try to treat warnings as errors so I'm conditioned to worry about them, idk.

@bpholt
Copy link

bpholt commented Aug 9, 2017

Following up on earlier—RC2 looks good AFAICT, other than the previously mentioned eviction warnings.

@muuki88 muuki88 merged commit 8215a4c into master Aug 10, 2017
@muuki88 muuki88 deleted the wip/cross-build-sbt-1.0 branch August 10, 2017 18:02
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.

4 participants