Maven: pacmak uses peerDependency instead of dependency/devDependency for Maven dependency
- Dominant language
- TypeScript
- Stars
- 2.9k
- Forks
- 267
- Avg merge
- 1d 25m
- Merged PRs (30d)
- 14
Description
Currently, `jsii-pacmak` uses the version range in `peerDependencies` to generate dependencies for Maven versions of libraries.
The way this works in NPM is:
* `peerDependencies`: advertise what version range of package we are compatible with
* `devDependencies`: install exactly this version to compile and test against
Maven, however, doesn't have the concept of an "advertised compatibility range". There is only one kind of ``, and it is of the "install and compile against this" kind. Version compatibility is not declared formally but communicated out-of-band: either by documentation or by convention.
Library consumers always have the opportunity to override transitive dependencies, because the dependency tree can only contain one copy of every package, so the problem that `peerDependencies` is trying to solve in NPM-land doesn't exist in Maven-land anyway.
### The problem
In practice, `peerDependencies` should always contain a range, which means in our current translation the Maven dependency always contains a range, of which Maven will always install the highest possible version. This has the following problems:
- Breaking changes in newer versions (which shouldn't happen, but can happen in parts of the API we don't control, i.e. CloudFormation resources) immediately break the build
- Technically we're not compiling against the right version: we should be compiling against the lowest version, not the highest, to make sure we're not accidentally taking a dependency on APIs that have been introduced in a higher version than our declared one. *This last risk is mitigated a bit by the fact that we already compiled and tested against the right, low version in TypeScript*.
- According to the report, Maven will download all versions in the version range, instead of just a single one. Have not observed this myself but I do believe it; `pip` does the same thing.
### Proposed change
Instead, we should probably do the following:
- We will still take public dependencies only from `dependencies`/`peerDependencies`.
- However, for every `peerDependency`, we will check whether it is *also* declared in `devDependencies`, and if so take its version range from there.
### Backwards compatibility impact
If we make this change, Maven will start installing the lowest declared version of all peerDependencies, instead of the highest. This will lead to problems in the following scenario:
```
lazily avoids declaring
a dependency on CDK
directly
v
┌─────────┐ ┌─────────┐ ┌─────────┐
│ │ │ jsii │ │ │
│ App │─────────▶│ library │────────▶│ CDK │
│ │ │ │ │ │
└─────────┘ └─────────┘ └─────────┘
^
used to bring in newest
CDK, will now bring in
old CDK
```
The App builder could declare a dependency on the library, and that would automatically always bring in the newest CDK if the App builder didn't declare a dependency on CDK directly. After this change, this will now bring in an old version (say, `2.100.0`, instead of the newest `2.184.0`). If the App builder had taken a dependency on newer APIs, their build would now be broken.
This is not something that *should* have been done, but as far as I am aware it is something that *can* be done, so it *will have been* done.
That means we can't change this behavior outright, and we will have to put it behind flags. Since the flag will be obscure, not a lot of people will know about it and enable it. The impact of any effort we put into this will therefore be low, so unless a lot of people are affected by the the current behavior that need access to a fix, the current classification of this issue will be a `p2`: low impact, workaround available by means of scripting a change to the generated `pom.xml` file.
----------------------------------------
# Original report
### Describe the bug
The bug has only reared its head due to a breaking change that was introduced in a new version of the `aws-cdk-lib` version `2.173.0`. However, this behavior would have been occurring so long as we have been using a `^` to denote our version number.
The issue arises due to defining a version in our `peerDependencies` for `aws-cdk-lib` as `^2.164.1`.
The following `jsii` logic fires:
1. Creates the pom file with dependencies [who need versions](https://github.com/aws/jsii/blob/acf7cd0708f43799e29601ccb0ac9bc419375ee2/packages/jsii-pacmak/lib/targets/java.ts#L1300).
2. Calls the `toMavenVersionRange` [function](https://github.com/aws/jsii/blob/acf7cd0708f43799e29601ccb0ac9bc419375ee2/packages/jsii-pacmak/lib/targets/version-utils.ts#L14) that calls the `toBracketNotation` function.
3. Utilizes the `>=` logic since `^` is logically equivalent and returns the [incorrect versioning scheme](https://github.com/aws/jsii/blob/acf7cd0708f43799e29601ccb0ac9bc419375ee2/packages/jsii-pacmak/lib/targets/version-utils.ts#L220-L222).
That logic produces the following pom entry:
```java
software.amazon.awscdk
aws-cdk-lib
[2.164.1,3.0.0)
```
This poses 2 discrete problems:
1. More code is being downloaded than necessary when running `mvn clean install` for compiling the jsii generated code. This is largely dependent on the range however. Construct has hundreds of versions between our current version and the next while the aws-cdk-lib only has a handful.
2. Unintended code source compilation. We intended the backing library to be `1.641.1` but instead it is using latest.
### Regression Issue
- [ ] Select this option if this issue appears to be a regression.
### Expected Behavior
We want the compiled java in our library to use the exactly defined version instead of latest. In other words:
```
software.amazon.awscdk
aws-cdk-lib
2.164.1
```
### Current Behavior
Packaging of the java artifact fails locally and on the pipeline with the following error(s):
```
14:14:58 #STDOUT> [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project redactedprojectname: Compilation failure: Compilation failure:
14:14:58 #STDOUT> [ERROR] redactedpath/file.java:[38,65] error: getHealthCheck() in InternalRecordSetProps clashes with getHealthCheck() in RecordSetOptions
14:14:58 #STDOUT> [ERROR] return type Object is not compatible with IHealthCheck
14:14:58 #STDOUT> [ERROR] redactedpath/file.java:[376,38] error: getHealthCheck() in Jsii$Proxy cannot implement getHealthCheck() in RecordSetOptions
14:14:58 #STDOUT> [ERROR] return type Object is not compatible with IHealthCheck
```
The version defined in peer dependencies does not have the new IHealthCheck interface. That interface was added on a later version. Our library uses JSii to package to all supported languages, except for C#, and Java is the only one that raises an issue.
### Reproduction Steps
It's hard to provide all of the setup without sharing internal configurations, but we basically just run:
```
jsii-pacmak -v --target java
```
### Possible Solution
At the moment, we removed the use of `^` from our library's peer dependencies. This locked us to a static version, and prevents the issue from happening. However, this is not an acceptable long term solution, as it creates conflicts with other internal libraries that extend ours.
### Additional Information/Context
_No response_
### SDK version used
jsii@5.1.12, jsii-pacmak@1.95.0, jsii-rosetta@5.1.24
### Environment details (OS name and version, etc.)
Behavior happens locally (MacOS Sonoma) and on the pipeline (Linux)
Contributor guide
Research direction
Start with packages/jsii-pacmak/lib/targets/java.ts and trace its dependency handling into packages/jsii-pacmak/lib/targets/version-utils.ts, then run `jsii-pacmak -v --target java` using the reported dependency setup. Compare the generated pom.xml version with the peerDependencies and devDependencies values; done should establish the intended flag-controlled behavior and prevent the incorrect Maven range from selecting an incompatible version.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java, typescript
- Domain
- build-system, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100