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

Java init template #229

Merged
merged 3 commits into from
Jul 3, 2018
Merged

Java init template #229

merged 3 commits into from
Jul 3, 2018

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jul 3, 2018

The template includes a java shim for the toolkit called "app.sh". The shim depends on a file .classpath.txt that's generated by maven during build. It points to target/classes for classpath. This supports a workflow where you can iterate in the IDE (and run unit tests), but then go to the command line to execute the toolkit, and you will target the same classes the IDE compiled.

BETA: the template includes a pom.xml file that depends on ~/.cdk/repo/maven as a pragmatic solution for beta. There's value in adding the repository in the pom.xml file directly as this gets us seamless IDE experience that works out of the box.

Elad Ben-Israel added 2 commits July 3, 2018 03:53
Include the pom.xml file in the maven repo, so that transitive dependencies will be respected.

This also required us to clean up the file and not include the path to the jsii-runtime repo. Instead, we simply deploy it into ~/.m2 during `./generate.sh`.

Not ideal, but works for now. When jsii is published to Maven Central, this is all going to go away, so trying to not over-engineer it.
The template includes a java shim for the toolkit called "app.sh". The shim depends on a file `.classpath.txt` that's generated by maven during build. It points to `target/classes` for classpath. This supports a workflow where you can iterate in the IDE (and run unit tests), but then go to the command line to execute the toolkit, and you will target the same classes the IDE compiled.

BETA: the template includes a pom.xml file that depends on `~/.cdk/repo/maven` as a pragmatic solution for beta. There's value in adding the repository in the pom.xml file directly as this gets us seamless IDE experience that works out of the box.
@@ -3,6 +3,11 @@ set -euo pipefail

mkdir -p project

# deploy jsii-runtime to the local maven repo so it will be discoverable
jsii_runtime_repo=$(node -e "console.log(path.join(path.dirname(require.resolve('jsii-java-runtime')), 'maven-repo'))")
Copy link
Contributor

Choose a reason for hiding this comment

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

This path will be that of whatever file the main field of package.json points to, which can be considered an implementation detail. I would advise against depending on that. Using the path to require.resolve('jsii-java-runtime/package.json') would be better. And better yet would be making jsii-java-runtime have an actual JS function or property that provides the path to the repository root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Changed to require.resolve('jsii-java-runtime/package.json')

@@ -3,6 +3,11 @@ set -euo pipefail

mkdir -p project

# deploy jsii-runtime to the local maven repo so it will be discoverable
jsii_runtime_repo=$(node -e "console.log(path.join(path.dirname(require.resolve('jsii-java-runtime')), 'maven-repo'))")
mkdir -p ~/.m2/repository
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not like how you're going to be messing with my ~/.m2 as part of this. There could be things in there that will interfere with the build, and this could cause the build to interfere with some of my things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the commit message, this is going away very soon once we publish jsii to Maven Central. For now, this seems like a pragmatic option.

@@ -0,0 +1,3 @@
{
"app": "./app.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt this is windows-friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😢 you are probably correct. I'll add to #138


public class HelloStackTest {
private static ObjectMapper JSON = new ObjectMapper()
.configure(SerializationFeature.INDENT_OUTPUT, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this way to indent the chain... Would prefer (but no hard feelings):

private static ObjectMapper JSON =
    new ObjectMapper().configure(SerializationFeature.INDENT_OUTPUT, true);

Also, should be private final static

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@eladb eladb merged commit 44b0b2a into master Jul 3, 2018
@eladb eladb deleted the benisrae/java-init-template branch July 8, 2018 19:00
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants