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

feat: initial jsii-srcmak work for golang #342

Closed
wants to merge 1 commit into from

Conversation

campionfellin
Copy link
Contributor

Starting off with some golang stuff here... some interesting issues with the moduleKey that I haven't seen before.. it seems to be required input (rather than optional previously).

JSII I think gets the output directory from name here:

https://github.com/aws/jsii-srcmak/blob/37bc954923b8b5dd076eec8412ffe646c1dca4be/src/compile.ts#L54-L57

And in JSII:

https://github.com/aws/jsii/blob/32c0add5edd0ed57d535241b483168e2b7e731ce/packages/jsii-pacmak/lib/targets/go.ts#L146-L151

Which somewhat makes sense, but when using the jsii-srcmak CLI, you usually don't put in the moduleKey explicitly, ending with it being a random hash:

https://github.com/aws/jsii-srcmak/blob/master/src/compile.ts#L29

So my dist folder looks like:

dist/go/4c6b576fbe7e27053874813d9754cb2e46811d806c1e0aa9aae8add4c3763060

I think JSII should probably base the outdir on moduleName input... will poke around there to see what I can do

Signed-off-by: campionfellin campionfellin@gmail.com

Signed-off-by: campionfellin <campionfellin@gmail.com>
if (!outdir) { throw new Error('--golang-outdir is required'); }
if (!module) { throw new Error('--golang-module is required'); }
return {
// moduleKey: module,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where you could explicitly set moduleKey for golang but then it breaks other languages

Copy link
Contributor

@ansgarm ansgarm May 14, 2021

Choose a reason for hiding this comment

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

Hey @campionfellin, could you elaborate on how it breaks the other languages? I tried it locally and it seemed to still work. I'd think it shouldn't matter to JSII whether there's a random hash or some actual name if the result does not contain that hash/name anyway? Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't quite remember here... I think it had something to do with moduleKey needing to be a random hash for some languages, but needed to be named for go. Perhaps I just couldn't figure it out at the time, but now it sounds like it's working for you!

Base automatically changed from master to main February 9, 2021 05:12
@ansgarm ansgarm mentioned this pull request May 14, 2021
@eladb eladb added effort/medium Medium work item – a couple days of effort p1 labels May 16, 2021
mergify bot pushed a commit that referenced this pull request May 20, 2021
This PR extends the work done in #342 by @campionfellin

JSII now supports specifying a `packageName` for Golang (which resolves the issues Campion mentioned in his [PR](#342 (comment))). I added `packageName` as a required parameter for this library (although it [isn't required](https://aws.github.io/jsii/user-guides/lib-author/configuration/targets/go/) when using JSII directly).  

I also looked into passing the generated `moduleKey` from `compile()` back to `srcmak()` but ditched that completely as the generated Go package would be named with the generated hash. And I don't think anyone would actually want to use it that way.
So I decided to keep it simple and make the `packageName` required instead.
@eladb
Copy link
Contributor

eladb commented May 21, 2021

Superseded by #476

@eladb eladb closed this May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/medium Medium work item – a couple days of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants