-
Notifications
You must be signed in to change notification settings - Fork 245
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
Produce independent mvn packages for java bindings #98
Conversation
…ter having moved the runtime compliance tests to a different package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dude, pleasure to read this. Elegant, clean, simple. Great job!
See some comments, nothing major.
set -euo pipefail | ||
|
||
mkdir -p conf | ||
/usr/bin/env node ./user.xml.t.js > conf/user.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between just running node
and /usr/bin/env node
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good Q. Honestly I just copied the style that was in the pom.xml.t.js
file without thinking much about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no technical difference /usr/bin/env
is useful for shebangs to actually be able to run a program that's discoverable in PATH (because shebangs need a program as the first arg).
.version(VERSION) | ||
.argv; | ||
|
||
const target = new (await Target.all)[argv.target]({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that says argv.target is ensured to be valid by yargs "choices"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
const force = argv.force; | ||
const packageDir = argv._[0]; | ||
await target.generateCode(codeDir); | ||
if (!argv.codeOnly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would bail-out early in the special-case: if (argv.codeOnly) { return; }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ I initially wrote this assuming there would be something after build that could be relevant also if codeOnly
, but that feels highly improbable now.
'modelVersion': '4.0.0', | ||
'name': `Java bindings for ${assm.name}`, | ||
|
||
...assm.targets.java.maven, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice leak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the nice things of ES6 :)
|
||
this.code.openFile('pom.xml'); | ||
this.code.line( | ||
xmlbuilder.create({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very elegant ❤️
} | ||
|
||
public async build(srcDir: string, outDir: string) { | ||
await fs.copy(srcDir, outDir, { overwrite: true, recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For overwrite
, yes, but the documentation makes no mention of recursive
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, isn't this the default for Target
, so you don't really need to override build
here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It suppresses the message saying there's no "build phase" for the "PackOnly" target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... kinda ugly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I guess I could remove the warning from the default... What do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps make build
abstract and supply a protected
utility for copySource
so implementors will explicitly indicate this is their intention
@@ -0,0 +1,120 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't there a comment about this is generated code somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is part of a tree that, axiomatically, contains only generated code... If I label that one file, I should also label every other file...
</properties> | ||
<dependencies> | ||
<dependency> | ||
<groupId>com.amazonaws</groupId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the groupId
for jsii-java-runtime
to com.amazonaws.jsii
(requested by the open source group)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey - that's super duper easy now :P
import path = require('path'); | ||
|
||
export const maven = { | ||
groupId: 'com.amazonaws', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to com.amazonaws.jsii
and resolve #102 (needed for dev preview)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
@@ -0,0 +1,15 @@ | |||
#!/bin/bash | |||
set -euo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a little comment what this script is about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
Fist step towards #73:
jsii-pacmak
produce a full maven project for each module being builtjsii-pacmak
build the resulting java project (unless--only-source
is passed)jsii-pacmak
publish the artifacts (or source if--only-source
is passed) to the--outdir
jsii-java-runtime
and related compliance tests