You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2022/10/24 05:38:32 UTC
[GitHub] [tomcat-jakartaee-migration] DanielThomas opened a new pull request, #36: Improve composability when using from other tools
DanielThomas opened a new pull request, #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36
It'd be incredibly useful to be able to compose this tool into different build workflows, so I've made some minor changes to make it easier to compose into other tools:
- When calling `Migration.execute`, return if any conversion was necessary, so we can use the untouched file if no conversion was necessary
- Allow custom profiles to be provided via an interface. I found that including `javax.annotation` was too broad, and we can stick with `javax` there as Spring 6 will continue to support the javax Pre/Post construct annotations; we have a _lot_ of libraries that embed those
- Allow the default excludes to be disabled so we can configure excludes ourselves
## Example Gradle Project
In our case, we want to use this inside [Gradle Artifact Transforms](https://docs.gradle.org/current/userguide/artifact_transforms.html), because they run at dependency resolution time, so affect compilation, test and IDEs, and allows these to happen transparently at build time.
Here's a standalone Gradle proof-of-concept. Once this lands, I'll follow up with a OSS Gradle plugin under our https://github.com/nebula-plugins/ project that'll provide Gradle conventions for configuring this transform.
```
buildscript {
dependencies {
classpath 'org.apache.tomcat:jakartaee-migration:1.0.5-SNAPSHOT'
}
repositories {
mavenLocal()
}
}
/* Example project */
plugins {
id 'java'
}
repositories {
mavenCentral()
}
dependencies {
implementation 'ch.qos.reload4j:reload4j:1.2.22' // javax.mail
implementation 'com.google.guava:guava:31.1-jre' // JSR-305 javax.annotation references should not be migrated
}
tasks.register('runtimeClasspathFiles') {
def files = configurations.runtimeClasspath.files
println '\nRuntime classpath files:'
files.each { println it }
}
/* Artifact transform based migration */
def artifactType = Attribute.of('artifactType', String)
def jakartaee = Attribute.of('jakartaee', Boolean)
configurations.all {
attributes.attribute(jakartaee, true)
}
dependencies {
attributesSchema {
attribute(jakartaee)
}
artifactTypes.named('jar') {
attributes.attribute(jakartaee, false)
}
registerTransform(JakartaEEMigration) {
from.attribute(jakartaee, false).attribute(artifactType, 'jar')
to.attribute(jakartaee, true).attribute(artifactType, 'jar')
}
}
import java.nio.file.Files
import java.util.regex.Pattern
import org.apache.tomcat.jakartaee.Migration
import org.apache.tomcat.jakartaee.EESpecProfile
import org.gradle.api.artifacts.transform.TransformAction
import org.gradle.api.artifacts.transform.TransformParameters
abstract class JakartaEEMigration implements TransformAction<TransformParameters.None> {
@PathSensitive(PathSensitivity.NAME_ONLY)
@InputArtifact
abstract Provider<FileSystemLocation> getInputArtifact()
@Override
void transform(TransformOutputs outputs) {
def inputFile = inputArtifact.get().asFile
def tempFilePath = Files.createTempFile("jakartaee", "transform")
Migration migration = new Migration()
migration.setSource(inputFile)
migration.setDestination(tempFilePath.toFile())
def tomcatWithoutAnnotations = Pattern.compile("javax([/\\.](annotation[/\\.]processing" +
"|ejb" +
"|el" +
"|mail" +
"|persistence" +
"|security[/\\.]auth[/\\.]message" +
"|servlet" +
"|transaction(?![/\\.]xa)" +
"|websocket))")
migration.setEESpecProfile(new EESpecProfile() {
@Override
String getSource() {
return 'javax'
}
@Override
String getTarget() {
return 'jakarta'
}
@Override
Pattern getPattern() {
return tomcatWithoutAnnotations
}
})
migration.setEnableDefaultExcludes(false)
if (migration.execute()) {
def nameWithoutExtension = inputFile.name.substring(0, inputFile.name.length() - 4) // these are always .jar
def outputFile = outputs.file("${nameWithoutExtension}-jakartaee.jar")
Files.move(tempFilePath, outputFile.toPath())
println "Converted $inputFile to JakartaEE $outputFile"
outputs.file(outputFile)
} else {
println "No JakartaEE transformation required for $inputFile"
Files.delete(tempFilePath)
outputs.file(inputFile)
}
}
}
```
Run `./gradlew runtimeClasspathFiles` to see the transforms in action:
```
No JakartaEE transformation required for /Users/dannyt/.gradle/caches/modules-2/files-2.1/com.google.guava/listenablefuture/9999.0-empty-to-avoid-conflict-with-guava/b421526c5f297295adef1c886e5246c39d4ac629/listenablefuture-9999.0-empty-to-avoid-conflict-with-guava.jar
No JakartaEE transformation required for /Users/dannyt/.gradle/caches/modules-2/files-2.1/com.google.guava/failureaccess/1.0.1/1dcf1de382a0bf95a3d8b0849546c88bac1292c9/failureaccess-1.0.1.jar
No JakartaEE transformation required for /Users/dannyt/.gradle/caches/modules-2/files-2.1/com.google.j2objc/j2objc-annotations/1.3/ba035118bc8bac37d7eff77700720999acd9986d/j2objc-annotations-1.3.jar
No JakartaEE transformation required for /Users/dannyt/.gradle/caches/modules-2/files-2.1/com.google.errorprone/error_prone_annotations/2.11.0/c5a0ace696d3f8b1c1d8cc036d8c03cc0cbe6b69/error_prone_annotations-2.11.0.jar
No JakartaEE transformation required for /Users/dannyt/.gradle/caches/modules-2/files-2.1/com.google.code.findbugs/jsr305/3.0.2/25ea2e8b0c338a877313bd4672d3fe056ea78f0d/jsr305-3.0.2.jar
No JakartaEE transformation required for /Users/dannyt/.gradle/caches/modules-2/files-2.1/org.checkerframework/checker-qual/3.12.0/d5692f0526415fcc6de94bb5bfbd3afd9dd3b3e5/checker-qual-3.12.0.jar
Converted /Users/dannyt/.gradle/caches/modules-2/files-2.1/ch.qos.reload4j/reload4j/1.2.22/f9d9e55d1072d7a697d2bf06e1847e93635a7cf9/reload4j-1.2.22.jar to JakartaEE /Users/dannyt/.gradle/caches/transforms-3/c890cd9b691a94575775292c738cbf45/transformed/reload4j-1.2.22-jakartaee.jar
No JakartaEE transformation required for /Users/dannyt/.gradle/caches/modules-2/files-2.1/com.google.guava/guava/31.1-jre/60458f877d055d0c9114d9e1a2efb737b4bc282c/guava-31.1-jre.jar
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005164436
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
Pushed a new commit which should address these concerns: https://github.com/apache/tomcat-jakartaee-migration/pull/36/commits/136c537e1e9d0274261d8749fe3a6747f6a9dc3c.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005151612
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
> some jars are converted, some are not, but information for the unconverted jars is changed
`Attributes` is a `HashMap` so many cases, reading and writing the manifest in this way is causing the key order to change, which is why I switched to always writing `destManifest`. If the intention was to write the manifest unchanged, we'd write original bytes instead, and use another way to determine if only implementation version was changed for the conversion return value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1004088967
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -112,7 +107,7 @@ private boolean updateValues(Attributes attributes, EESpecProfile profile) {
if (attributes.containsKey(Attributes.Name.IMPLEMENTATION_VERSION)) {
String newValue = attributes.get(Attributes.Name.IMPLEMENTATION_VERSION) + "-" + Info.getVersion();
attributes.put(Attributes.Name.IMPLEMENTATION_VERSION, newValue);
- result = true;
+ // Purposefully avoid setting result
Review Comment:
Changing the version also counts as a conversion, so here need to revert.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] rmaucher commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
rmaucher commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1290577491
This looks like a very good improvement overall.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1291681450
Merge manually, thanks for the PR. ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005100601
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -112,7 +107,7 @@ private boolean updateValues(Attributes attributes, EESpecProfile profile) {
if (attributes.containsKey(Attributes.Name.IMPLEMENTATION_VERSION)) {
String newValue = attributes.get(Attributes.Name.IMPLEMENTATION_VERSION) + "-" + Info.getVersion();
attributes.put(Attributes.Name.IMPLEMENTATION_VERSION, newValue);
- result = true;
+ // Purposefully avoid setting result
Review Comment:
This is metadata and would cause every file to be considered converted, even if no interesting conversions took place. The destination manifest is written regardless with this change. Omitting this is what allows `hasConverted()` to work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005164436
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
Pushed a new commit which should hopefully address these concerns: https://github.com/apache/tomcat-jakartaee-migration/pull/36/commits/136c537e1e9d0274261d8749fe3a6747f6a9dc3c.
To clarify, the intention of `hasConverted` is not to indicate that no changes were made, but that the source can be used and satisfy the selected profile. Manifests still get modified, zip version compatibility changes, file attributes are dropped, etc., that's unavoidable with the current implementation, which makes it impossible to tell if you need to use a destination file, which is why `hasConverted` needs to exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1291438441
Awesome, thanks much for the feedback! It's a much better PR for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1288571178
I like this PR, but I don't like the change of the Migration class as this change made a bit numerous and a bit hassle. I think we could add a field to identify whether has been converted, like this`migration.hasConverted()`, which requires only minor changes to the implementation of the` Migration#migrateStream` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1002897456
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
In testing, I found that writing the source manifest doesn't guarantee the order of attributes, so the manifest differs even with no value changes (comparing using `pkgdiff`). This and the change below allows the implementation version to change only when other conversions were also made.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005100601
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -112,7 +107,7 @@ private boolean updateValues(Attributes attributes, EESpecProfile profile) {
if (attributes.containsKey(Attributes.Name.IMPLEMENTATION_VERSION)) {
String newValue = attributes.get(Attributes.Name.IMPLEMENTATION_VERSION) + "-" + Info.getVersion();
attributes.put(Attributes.Name.IMPLEMENTATION_VERSION, newValue);
- result = true;
+ // Purposefully avoid setting result
Review Comment:
This is metadata and would cause every file with an implementation version attribute (which is practically everything) to be considered converted, even if no interesting conversions took place. The destination manifest is written regardless with this change, so this doesn't prevent the updated implementation version from hitting the destination file. Omitting this is what allows `hasConverted()` to work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005164436
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
Pushed a new commit which should hopefully address these concerns: https://github.com/apache/tomcat-jakartaee-migration/pull/36/commits/136c537e1e9d0274261d8749fe3a6747f6a9dc3c.
To clarify, the intention of `hasConverted` is not to indicate that no changes were made, but that the source can be used and satisfy the selected profile. Manifests still get modified, zip version compatibility changes, file attributes are dropped, etc., that's unavoidable with the current implementation. That makes it impossible to tell if you need to use a destination file, which is why `hasConverted` exists.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1288655837
Hi @DanielThomas ,
> The migrator was stateless in terms of the result, so I figured that was a goal, which is why I went with method returns.
On the contrary, I think that if a state is needed, that state should be attributed to the migrator, since `source` and `destination` values need to be set before executing the execute method. And for the execute method, I think it either succeeds or fails, and the presence or absence of converted, as you need it to be, is not really logically relevant to the execute method.
> Would this method throw if execute() hasn't yet been called?
Yes.
> What should happen if execute() is called more than once for a given instance of the migrator?
If the state of a given migrator with `source` and `destination` unchanged should remain the same as the state of the previous execution.
BTW, for the use of migrators, I believe that a task should correspond to a migrator, rather than one migrator being used repeatedly to handle multiple tasks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1004090783
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
This change raises the problem that if a version number exists, it must not be the original version number, even if the rest of the content has not been converted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005151612
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
> some jars are converted, some are not, but information for the unconverted jars is changed
`Attributes` is a `HashMap` so in many cases, reading and writing the manifest in this way is causing the key order to change, which is why I switched to always writing `destManifest`. If the intention was to write the manifest unchanged, we'd write the original bytes instead, and use another way to determine if only implementation version was changed for the conversion return value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005142148
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
Here it is still necessary to revert, as the conversion is fine for the specified jar, but for directory migration via the migrator, the manifest of some jars here will be affected(some jars are converted, some are not, but information for the unconverted jars is changed), even if the jar is not converted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005100601
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -112,7 +107,7 @@ private boolean updateValues(Attributes attributes, EESpecProfile profile) {
if (attributes.containsKey(Attributes.Name.IMPLEMENTATION_VERSION)) {
String newValue = attributes.get(Attributes.Name.IMPLEMENTATION_VERSION) + "-" + Info.getVersion();
attributes.put(Attributes.Name.IMPLEMENTATION_VERSION, newValue);
- result = true;
+ // Purposefully avoid setting result
Review Comment:
This is metadata and would cause every file to be considered converted, even if no interesting conversions took place. The destination manifest is written regardless with this change, so this doesn't prevent the updated implementation version from hitting the destination file. Omitting this is what allows `hasConverted()` to work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005151612
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
> some jars are converted, some are not, but information for the unconverted jars is changed
`Attributes` is a `HashMap` so in many cases, reading and writing the manifest in this way is causing the key order to change, which is why I switched to always writing `destManifest`. If the intention was to write the manifest unchanged, we'd write the original bytes instead, and use another way to determine if only implementation version was changed for the conversion return value.
Edit: In fact, I'll do just that and it occurs to me that signatures should get the same handling. Let me get another commit up and see what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1289917738
All done here @aooohan. Let me know if there's anything else you'd like changed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] rmaucher commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
rmaucher commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1291700768
Tested with the examples webapp from Tomcat 9 and verified that it is a bit faster (about 20% for this one).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1289903755
Great, I'll make those changes!
> BTW, for the use of migrators, I believe that a task should correspond to a migrator, rather than one migrator being used repeatedly to handle multiple tasks.
Oh, I totally agree. Just wanted to make sure I clarified the intent :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005151612
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
> some jars are converted, some are not, but information for the unconverted jars is changed
`Attributes` is a `HashMap` so in the majority of cases, reading and writing the manifest in this way is causing the key order to change, which is why I switched to always writing `destManifest`. If the intention was to write the manifest unchanged, we'd write original bytes instead, and use another way to determine if only implementation version was changed for the conversion return value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1005164436
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
Pushed a new commit which should hopefully address these concerns: https://github.com/apache/tomcat-jakartaee-migration/pull/36/commits/136c537e1e9d0274261d8749fe3a6747f6a9dc3c.
To clarify, the intention of `hasConverted` is not to indicate that no changes were made, but that the source can be used and satisfy the selected profile. Manifests still get modified, zip version compatibility changes, file attributes are dropped, etc., that's unavoidable with the current implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan commented on a diff in pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan commented on code in PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#discussion_r1004090783
##########
src/main/java/org/apache/tomcat/jakartaee/ManifestConverter.java:
##########
@@ -48,20 +48,16 @@ public boolean accepts(String filename) {
}
@Override
- public void convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
+ public boolean convert(String path, InputStream src, OutputStream dest, EESpecProfile profile) throws IOException {
Manifest srcManifest = new Manifest(src);
Manifest destManifest = new Manifest(srcManifest);
- boolean result = false;
-
- result = result | removeSignatures(destManifest);
+ boolean result = removeSignatures(destManifest);
result = result | updateValues(destManifest, profile);
- if (result) {
- destManifest.write(dest);
- } else {
- srcManifest.write(dest);
Review Comment:
This change raises the problem that if `IMPLEMENTATION_VERSION` exists, it must not be the original version content, even if the rest of the content has not been converted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] aooohan closed pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
aooohan closed pull request #36: Improve composability when using from other tools
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat-jakartaee-migration] DanielThomas commented on pull request #36: Improve composability when using from other tools
Posted by GitBox <gi...@apache.org>.
DanielThomas commented on PR #36:
URL: https://github.com/apache/tomcat-jakartaee-migration/pull/36#issuecomment-1288624526
The migrator was stateless in terms of the result, so I figured that was a goal, which is why I went with method returns.
Would this method throw if execute() hasn't yet been called? What should happen if execute() is called more than once for a given instance of the migrator?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org