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

chore: enforce assembly execution order #1076

Closed
wants to merge 4 commits into from

Conversation

Roiocam
Copy link
Member

@Roiocam Roiocam commented Jan 29, 2024

Motivation

  • Avoid repeatedly modifying packageBin semantics
  • Avoid executing assembly on other module
  • Enforce dependency order: osgiBundle -> assembly -> compile

Verified

Execute sbt protobuf-v3 / packageBin, and then we will be observed the execution result like assembly -> osgiBundle.

It won't break assembly when executing packageBin or osgiBundle.

截屏2024-01-29 17 01 06

@Roiocam Roiocam marked this pull request as draft January 29, 2024 09:25
@Roiocam
Copy link
Member Author

Roiocam commented Jan 29, 2024

Draft because sbt-depend-walker not publish yet.

@mdedetrich
Copy link
Contributor

@Roiocam Have you tried the sbt-osgi caching option, i.e. sbt/sbt-osgi#100 ? I believe this was explicitly designed to solve this issue and may be less heavy handed?

@Roiocam
Copy link
Member Author

Roiocam commented Jan 29, 2024

@Roiocam Have you tried the sbt-osgi caching option, i.e. sbt/sbt-osgi#100 ? I believe this was explicitly designed to solve this issue and may be less heavy handed?

Thanks, I have enabled this, it will reduce the execution of the osgiBundle.

@Roiocam Roiocam marked this pull request as ready for review January 29, 2024 16:27
@Roiocam Roiocam changed the title Reduce the execution of assembly and enforce dependency order enforce assembly execution order Jan 29, 2024
@Roiocam Roiocam changed the title enforce assembly execution order chore: enforce assembly execution order Jan 29, 2024
@Roiocam Roiocam mentioned this pull request Jan 29, 2024
32 tasks
@mdedetrich
Copy link
Contributor

@Roiocam So I don't want to stop the PR, but I just want to make you aware (if you aren't already) that there generally is an issue with how sbt-osgi interacts with other packaging type of tasks i.e. ssembly,multi-release-jar/jdk9 etc etc when it comes to ordering and there are long discussions on this i.e.

The reason why I am pointing this out is that it may make sense to take a step back and resolve this problem in a principled way within sbt-osgi (although this can be done later!). For example as a quick idea that came to the top of my head, it makes sense to add the capability of adding sbt Configuration's aka Config's to sbt-osgi in a similar way that is done with sbt-header so that sbt-osgi is aware of other configs and then can handle ordering issues accordingly/correctly

Also note I am a maintainer on sbt-osgi so if you open up PR's there with tests I can review them

@Roiocam
Copy link
Member Author

Roiocam commented Jan 30, 2024

@Roiocam So I don't want to stop the PR, but I just want to make you aware (if you aren't already) that there generally is an issue with how sbt-osgi interacts with other packaging type of tasks i.e. ssembly,multi-release-jar/jdk9 etc etc when it comes to ordering and there are long discussions on this i.e.

How should we solve this problem? In my opinion, the real issue is sbt-osgi overwrited the Task( Compile / packageBin), and not calling (Compile / fullClasspath). JDK9 plugin broken by this, and sbt-assembly too.

Why sbt-assembly doesn't overwrite packageBin but instead let user choose? I think the sbt-osgi should be doing this too.

As for fullClasspath blocker issue, i think this sbt/sbt-osgi#103 doesn't help. Why not change like this:

  lazy val defaultOsgiSettings: Seq[Setting[_]] = {
    import OsgiKeys._
    Seq(
+     (Compile / exportJars) := false,
      bundle := Osgi.bundleTask(
        manifestHeaders.value,
        additionalHeaders.value,
 -      (Compile / dependencyClasspathAsJars).value.map(_.data) ++ (Compile / products).value,
+       (Compile / fullClasspath).value.map(_.data) ++ (Compile / internalDependencyAsJars).value.map(_.data),
        (Compile / packageBin / artifactPath).value,
        (Compile / resourceDirectories).value,
        embeddedJars.value,
        explodedJars.value,
        failOnUndecidedPackage.value,
        (Compile / sourceDirectories).value,
        (Compile /  packageBin / packageOptions).value,
        packageWithJVMJar.value,
        cacheStrategy.value,
        streams.value),
-     Compile / sbt.Keys.packageBin := bundle.value,

Let me explain that osgiBundle itself will be packaged thing into the jar, so we could avoid the task graph like packageBin -> osgiBundle -> fullClasspath -> exportProduct -> packageBin.

As for internalDependencyAsJars, because fullClasspath doesn't contain manifest. And the dependencyClasspathAsJars just a concat of internalDependencyAsJars and externalDependencyClasspath.

So we could replace them. This should fix the issues of JDK9 and assembly not executing issue, because when we execute osgiBundle, we will execute compile/fullClasspath.

@Roiocam
Copy link
Member Author

Roiocam commented Jan 30, 2024

So we could replace them. This should fix the issues of JDK9 and assembly not executing issue, because when we execute osgiBundle, we will execute compile/fullClasspath.

I haven't verified yet.

@He-Pin He-Pin added the build sbt or build of the project label Jan 30, 2024
@Roiocam
Copy link
Member Author

Roiocam commented Jan 30, 2024

So we could replace them. This should fix the issues of JDK9 and assembly not executing issue because when we execute osgiBundle, we will execute compile/fullClasspath.

Yes, I have confirm this, and the manifest has been keep too.

> diff newpb/META-INF/MANIFEST.MF oldpb/META-INF/MANIFEST.MF
5c5
< Bnd-LastModified: 1706633706879
---
> Bnd-LastModified: 1706633635527
7c7
<  0-8a759bad20240131-0054-SNAPSHOT",org.apache.pekko.protobufv3.internal.
---
>  0-8a759bad20240130-2351-SNAPSHOT",org.apache.pekko.protobufv3.internal.
9c9
<  310-8a759bad20240131-0054-SNAPSHOT"
---
>  310-8a759bad20240130-2351-SNAPSHOT"
15c15
< Specification-Version: 1.1.0-M0+310-8a759bad+20240131-0054-SNAPSHOT
---
> Specification-Version: 1.1.0-M0+310-8a759bad+20240130-2351-SNAPSHOT
22,23c22,23
< Bundle-Version: 1.1.0.M0310-8a759bad20240131-0054-SNAPSHOT
< Implementation-Version: 1.1.0-M0+310-8a759bad+20240131-0054-SNAPSHOT
---
> Bundle-Version: 1.1.0.M0310-8a759bad20240130-2351-SNAPSHOT
> Implementation-Version: 1.1.0-M0+310-8a759bad+20240130-2351-SNAPSHOT
32c32
<  20240131-0054-SNAPSHOT
---
>  20240130-2351-SNAPSHOT
~

@Roiocam
Copy link
Member Author

Roiocam commented Feb 1, 2024

Blocked by sbt/sbt-osgi#131

@Roiocam Roiocam marked this pull request as draft February 20, 2024 16:21
@Roiocam Roiocam closed this Apr 11, 2024
@He-Pin
Copy link
Member

He-Pin commented Apr 11, 2024

@Roiocam You can change it to draft.

@Roiocam
Copy link
Member Author

Roiocam commented Apr 11, 2024

@Roiocam You can change it to draft.

I don't think this PR is necessary after reviewing #1125 and trying it on my local, so I closed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build sbt or build of the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants