aws / aws/jsii

Maven: pacmak uses peerDependency instead of dependency/devDependency for Maven dependency

Open
#4,811 4 comments 0 reactions 0 assignees View on GitHub
bug effort/medium p2
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.