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

CONTRIBUTING.md needs some minor updates in Getting Started / Pack #20041

Closed
jusdino opened this issue Apr 22, 2022 · 13 comments · Fixed by #20581
Closed

CONTRIBUTING.md needs some minor updates in Getting Started / Pack #20041

jusdino opened this issue Apr 22, 2022 · 13 comments · Fixed by #20581
Assignees
Labels
documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2

Comments

@jusdino
Copy link
Contributor

jusdino commented Apr 22, 2022

Describe the issue

Below is a long-winded way of saying:

Short version

What did you try?

Read CONTRIBUTING.md and follow its instructions for packaging a module in aws-cdk

What happened?

I was eventually able to run yarn run package but did not find any python packages being produced

What behavior did you expect:

To understand how to locally work in the repo then produce packages that I can use for local testing/development and (most importantly to me) understand how to do this for v2 of this lib.

Details

I'm trying to get ready to contribute some updates to this project but, before I dive in, I wanted to get familiar and get an idea of how I can try out local changes and package them to see if they will do what I need in a Python based app. From reading CONTRIBUTING.md, my understanding of how best to go from zero to python-package would be more or less (after installing node/yarn/.NET/python/docker:

$ git clone https://github.com/{your-account}/aws-cdk.git
$ cd aws-cdk
$ yarn install
$ cd packages/@aws-cdk/aws-ec2
$ ../../../scripts/buildup  # Unclear to me if this is necessary but it doesn't hurt
$ cd <root-of-cdk-repo>
$ docker run --rm --net=host -it -v $PWD:$PWD -w $PWD jsii/superchain
docker$ cd packages/@aws-cdk/aws-ec2
docker$ ../../../scripts/foreach.sh --up yarn run package

That last step is where things go wonky. I spent a few days off and on trying to get that to work and here is the short version of what I found:
Running things as described reliably produces an error for me:

aws-cdk/packages/aws-cdk: yarn run package  (12 remaining)
yarn run v1.22.5
$ cdk-package
Validating circular imports
Error: spawnSync .../aws-cdk/node_modules/esbuild-linux-64/bin/esbuild ENOBUFS
Package failed. Total time (2.5s)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error: last command failed. fix problem and resume by executing: .../dev/aws-cdk/packages/@aws-cdk/aws-ec2/../../../scripts/foreach.sh
directory:    .../aws-cdk/packages/aws-cdk

After a little digging, I found that the :latest version of the jsii docker referenced in CONTRIBUTING.md uses node10, while the docs say that >=14.15.0 is required. If I instead use :1-buster-slim-node14, that gets me further, but another change between versions is that :latest runs as root, while the latter does not, so it runs into a number of file permissions issues trying to operate on folders/files that the docker user does not own. The most reliable way I was able to get past that was to force the user back to root with the --user root arg for docker run (not ideal, obviously). So, changing the package part of the process, gets me a successful package run:

$ docker run --rm --net=host -it --user root -v $PWD:$PWD -w $PWD jsii/superchain:1-buster-slim-node14
docker$ cd packages/@aws-cdk/aws-ec2
docker$ ../../../scripts/foreach.sh --up yarn run package

But wait, there's more!

Looking at the command output, I was excited to go find my python packages that I could theoretically go test in a demo app, but to my surprise, there was js, dotnet, and java output in all the relevant dist/ folders, but nothing for python. Monitoring the /tmp/foreach.stdio file during another attempt showed me that it did 'finish' the python packaging, though I can find no artifacts of that having happened 😕 .

So that leaves me with two problems:

  1. I can't seem to produce a package for python in the module used as an example for CONTRIBUTING.md
  2. Once I manage that, I'm not sure how to go about doing the same for cdk v2

Links

CONTRIBUTING.md

@jusdino jusdino added documentation This is a problem with documentation. needs-triage This issue or PR still needs to be triaged. labels Apr 22, 2022
@peterwoodworth peterwoodworth added p1 effort/small Small work item – less than a day of effort labels Apr 22, 2022
@peterwoodworth peterwoodworth removed the needs-triage This issue or PR still needs to be triaged. label Apr 22, 2022
@peterwoodworth
Copy link
Contributor

@comcalvi @kaizen3031593 I'm not familiar with this process, could one of you help out here?

@kaizencc
Copy link
Contributor

I dont think either of us are familiar, rerouting to @RomainMuller who is my best guess for help here.

@kaizencc kaizencc assigned RomainMuller and unassigned kaizencc and comcalvi Apr 22, 2022
@kaizencc kaizencc added p2 and removed p1 labels Apr 22, 2022
@rabbittsoup
Copy link
Contributor

Anyone familiar?

@RomainMuller
Copy link
Contributor

The documentation should be updated to mention the :1-buster-slim tag instead of :latest for sure. Right now however this is still on node12 (this will be updated soon to be node14).

Regarding the absence of python packages, I will need to try and reproduce this.

@RomainMuller
Copy link
Contributor

Okay so, I was able to reproduce this locally... Now to understand what's actually going on...

  • yarn run package succeeds, but dist/python is not getting created
  • yarn jsii-pacmak -t python does create the expected dist/python directory
  • yarn jsii-pacmak (which is what yarn run package should boil down to) does create dist/python

It appears to reproduce consistently, and I guess the symptoms tend to point to an issue with cdk-package... But this ought to be a very simple/trivial wrapper around jsii-pacmak.

@RomainMuller
Copy link
Contributor

yarn run package --target=python also fails to produce the dist/python directory.

@RomainMuller
Copy link
Contributor

Ah-ha! yarn jsii-pacmak -t python -o dist also does not produce a dist/python directory. This might be a weird interplay with the -o flag that is being passed by cdk-package.

@RomainMuller
Copy link
Contributor

Alright - I found the culprit to be a bug in jsii-pacmak that, when passed a -o option, that would not be resolved up-front, and the Python builder would incorrectly use this within the context of it's temporary sourceDir, instead of using the context of the packageDir.

The solution is to resolve the -o path upfront so that it is made absolute from the beginning.


If you want to unblock yourself until the jsii-pacmak fix is released (hopefully very soon), you can edit the following line:

So that it instead reads:

'-o', path.resolve(process.cwd(), outdir)];

This way, the path passed to jsii-pacmak -o will be absolute and this should circumvent the bug.

RomainMuller added a commit to aws/jsii that referenced this issue May 5, 2022
When the `-o`/`--outdir` option is passed to `jsii-pacmak` with a
relative path, that path was not resolved up-front by the CLI, which
resulted in the Python code-generator not outputting the wheel files in
the expected directory: instead of being consistently treated as
relative to the current working directory, the Python generator
interpreted it relative to it's temporary working directory.

This addresses the issue by resolving relative paths to absolute paths,
using `process.cwd()` as the base directory, before passing the value
down to the business logic.

Fixes aws/aws-cdk#20041
RomainMuller added a commit to aws/jsii that referenced this issue May 5, 2022
When the `-o`/`--outdir` option is passed to `jsii-pacmak` with a
relative path, that path was not resolved up-front by the CLI, which
resulted in the Python code-generator not outputting the wheel files in
the expected directory: instead of being consistently treated as
relative to the current working directory, the Python generator
interpreted it relative to it's temporary working directory.

This addresses the issue by resolving relative paths to absolute paths,
using `process.cwd()` as the base directory, before passing the value
down to the business logic.

Fixes aws/aws-cdk#20041
@RomainMuller RomainMuller assigned kaizencc and comcalvi and unassigned RomainMuller May 5, 2022
@RomainMuller
Copy link
Contributor

I have made PRs to:

  • Address the bug in jsii-pacmak that causes Python artifacts to be missing
  • Update the CONTRIBUTING.md document to have the correct image tag

Regarding the aws-cdk@v2 question, this is no longer in my turf, so assigning back to @kaizencc and @comcalvi to see to have the CONTRIBUTING.md guide updated with the relevant instructions.

mergify bot pushed a commit that referenced this issue May 5, 2022
The `jsii/superchain` image no longer has a `:latest` tag and users need to expressly reference `:1-buster-slim` instead.

See #20041

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mergify bot pushed a commit to aws/jsii that referenced this issue May 5, 2022
)

When the `-o`/`--outdir` option is passed to `jsii-pacmak` with a
relative path, that path was not resolved up-front by the CLI, which
resulted in the Python code-generator not outputting the wheel files in
the expected directory: instead of being consistently treated as
relative to the current working directory, the Python generator
interpreted it relative to it's temporary working directory.

This addresses the issue by resolving relative paths to absolute paths,
using `process.cwd()` as the base directory, before passing the value
down to the business logic.

See aws/aws-cdk#20041



---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
@kaizencc
Copy link
Contributor

Sorry for playing hot potato @jusdino but we have an internal issue to update the contributing guide with v2 workflow changes with a target date of june 1 (when v1 goes into maintenance mode). Assigning back to @madeline-k and me who will make sure that happens on that day.

@kaizencc kaizencc assigned madeline-k and unassigned comcalvi May 12, 2022
@jusdino
Copy link
Contributor Author

jusdino commented May 12, 2022

No problems. For what it’s worth, I worked up a dockerfile/script that helps with some of the superchain user permission issues, at least. I’m using it to try to build v2 now and was considering opening a PR in case you wanted to add it to scripts.

@jusdino
Copy link
Contributor Author

jusdino commented May 12, 2022

Another interesting discovery I just made while trying to figure out how to build v2's aws-cdk-lib - it looks like @aws-cdk/lambda-layer-kubectl tries to use the docker daemon to build a lambda layer, which obviously won't work unless you mount the docker.sock into the superchain container - something CONTRIBUTING.md definitely doesn't currently mention:

yarn run v1.22.18
$ cdk-build
>> Building AWS Lambda layer inside a docker image...
Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
Error: layer/build.sh exited with error code 1
Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error: last command failed. fix problem and resume by executing: /home/jusdino/dev/aws-cdk/scripts/foreach.sh
directory:    /home/jusdino/dev/aws-cdk/packages/@aws-cdk/lambda-layer-kubectl
jusdino@hostname:~/dev/aws-cdk/packages/aws-cdk-lib$ tail: /tmp/foreach.stdio: file truncated
yarn run v1.22.18
$ cdk-build
>> Building AWS Lambda layer inside a docker image...
Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
Error: layer/build.sh exited with error code 1
Build failed.!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

wphilipw pushed a commit to wphilipw/aws-cdk that referenced this issue May 23, 2022
The `jsii/superchain` image no longer has a `:latest` tag and users need to expressly reference `:1-buster-slim` instead.

See aws#20041

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify mergify bot closed this as completed in #20581 Jun 2, 2022
mergify bot pushed a commit that referenced this issue Jun 2, 2022
…0581)

Now that v1 is in maintenance mode, we need some updates to the contributing guide. 

The biggest change to the developer workflow now that v1 is in maintenance mode, is that the main development branch, `main`, has the v2 source code. This does not have many practical impacts on developer workflow, because in most cases we will continue to just iterate on a single service module in its `packages/@aws-cdk/aws-<service>` directory.

This PR adds instructions for building and testing `aws-cdk-lib` and the individual `-alpha` packages, which developers might need to do occasionally. 

closes https://github.com/cdklabs/cdk-ops/issues/1933 

closes #20041 


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Jun 2, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
…s#20581)

Now that v1 is in maintenance mode, we need some updates to the contributing guide. 

The biggest change to the developer workflow now that v1 is in maintenance mode, is that the main development branch, `main`, has the v2 source code. This does not have many practical impacts on developer workflow, because in most cases we will continue to just iterate on a single service module in its `packages/@aws-cdk/aws-<service>` directory.

This PR adds instructions for building and testing `aws-cdk-lib` and the individual `-alpha` packages, which developers might need to do occasionally. 

closes https://github.com/cdklabs/cdk-ops/issues/1933 

closes aws#20041 


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants