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

fix: support hoisted jsii dependencies #644

Closed
wants to merge 8 commits into from

Conversation

MrArnoldPalmer
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer commented Mar 22, 2022

Removes copy of the project directory and instead only outputs
transliterated assemblies to the working directory. This avoids
potential problems when generating docs for packages that may have some
dependencies that are symlinks, such as those in a monorepo.

Pending aws/jsii#3437

fixes #390

Removes copy of the project directory and instead only outputs
transliterated assemblies to the working directory. This avoids
potential problems when generating docs for packages that may have some
dependencies hoisted. IE when building against `aws-cdk-lib` within
`node_modules` and `constructs` is hoisted to the top level
`node_modules` directory as well.

Pending aws/jsii#3437

fixes #390
@MrArnoldPalmer
Copy link
Contributor Author

NOTE: Originally this was targeting allowing users to transliterate assemblies for projects that may have hoisted dependencies, IE a dependency is within a parent directory's node_modules somewhere. However, just changing to using the package directory as the working directory and outputting the transliterated assemblies elsewhere isn't good enough, as the assembliesDir defaults to the project directory and wont search parent directories for dependencies/.jsii assemblies.

This however should still fix the resolution of symlinked dependencies that may exist in a packages node_modules such as those within a lerna managed monorepo. This case is slightly harder to setup a succint test for but open to suggestions if reviewers feel strongly about adding one.

@MrArnoldPalmer MrArnoldPalmer marked this pull request as ready for review June 8, 2022 19:49
@agdimech
Copy link
Contributor

agdimech commented Jun 15, 2022

This PR also improves performance by ~ 6x :)

One of my builds is down from 6 mins to 1 min with this PR.

In terms of the failing tests, this is caused by a transient error and jest timeout flakiness. Ran locally and all tests passed:

Test Suites: 14 passed, 14 total
Tests:       109 passed, 109 total
Snapshots:   132 passed, 132 total
Time:        506.923 s

@MrArnoldPalmer
Copy link
Contributor Author

yeah tests are fine for me as well but I haven't bothered trying to fix the flakiness in a different PR yet.

@agdimech
Copy link
Contributor

agdimech commented Jun 16, 2022

@Chriscbr - are we able to merge this or do tests have to pass here?

@MrArnoldPalmer
Copy link
Contributor Author

@agdimech tests definitely have to pass, there is a some non-zero chance I've broken something (maybe a race condition causing the discrepancy in local vs CI execution).

@agdimech agdimech mentioned this pull request Jun 17, 2022
agdimech added a commit to agdimech/jsii-docgen that referenced this pull request Jun 17, 2022
Removes copy of the project directory and instead only outputs
transliterated assemblies to the working directory. This avoids
potential problems when generating docs for packages that may have some
dependencies hoisted. IE when building against `aws-cdk-lib` within
`node_modules` and `constructs` is hoisted to the top level
`node_modules` directory as well.

Note: This fixes unit tests for
cdklabs#644. Full credit goes to
Mitchell Valine @MrArnoldPalmer.

Fixes: cdklabs#390
@agdimech
Copy link
Contributor

agdimech commented Jun 17, 2022

Couldn't figure out how to contribute to this PR so I opened a fresh one which should fix the unit tests: #714

Feel free to take the fix and apply it here and close mine or vise versa.

MrArnoldPalmer and others added 3 commits June 17, 2022 13:19
Removes copy of the project directory and instead only outputs
transliterated assemblies to the working directory. This avoids
potential problems when generating docs for packages that may have some
dependencies hoisted. IE when building against `aws-cdk-lib` within
`node_modules` and `constructs` is hoisted to the top level
`node_modules` directory as well.

Note: This fixes unit tests for
#644. Full credit goes to
Mitchell Valine @MrArnoldPalmer.

Fixes: #390
@MrArnoldPalmer
Copy link
Contributor Author

@agdimech ty! I didn't realize it was purely a timeout as I tried previously upping it and still seeing failures. I merged in your commit here just to prevent re-review etc. Should auto-merge when successful.

@agdimech
Copy link
Contributor

Sorry, I guess that didn’t fix it :(

I’ve seen this combination of errors locally to which I bumped the timeout and it resolved. Guess my hardware might be a bit beefier then the hw used as part of the build.

We can try bumping this to something like 120 although there may be a nasty timing issue contributing to this. What is strange is I would have thought that the test duration would have reduced with this PR, given the lack of fs.copy.

@agdimech
Copy link
Contributor

agdimech commented Jun 17, 2022

I just had a look at the other open PR: https://github.com/cdklabs/jsii-docgen/runs/6928391566?check_suite_focus=true

the same issue is showing up there related to:
ENOENT: no such file or directory, chdir '/tmp/EtnTZW' -> '/tmp/wryVeX . What is really strange is it is failing when trying to cd back to the previous cwd as per:

process.chdir(cwd);

Is it possible for this scenario to occur?

A) Simple Formatter Test Runs from cwd tmp/wryVeX and commences the work function .
B) Another test starts and create a workDir at tmp/wryVeX. This test completes and rms tmp/wryVeX
C) Simple Formatter Test completes and tries to cd back to cwd which no longer exists, hence the failure.

@RichiCoder1
Copy link

I'm manually applying this patch to a local installation and I hope it moves forward if only because the massive perf boost.

@agdimech
Copy link
Contributor

And thoughts on my previous comments?

I don’t think these build failures are related to this PR as per: https://github.com/cdklabs/jsii-docgen/runs/7122708633?check_suite_focus=true

cogwirrel added a commit to cogwirrel/aws-prototyping-sdk that referenced this pull request Jul 7, 2022
cogwirrel added a commit to aws/aws-pdk that referenced this pull request Jul 7, 2022
* feat(open-api-gateway): add documentation generation support

* perf(docs): hackily apply cdklabs/jsii-docgen#644 to avoid docs build timeout

This should be removed once cdklabs/jsii-docgen#644 is merged
@agdimech
Copy link
Contributor

Any idea how to proceed with this?

@RichiCoder1
Copy link

Plusing this again. Fix both perf issues and support for Yarn/PNP which hoist dependencies.

mergify bot pushed a commit that referenced this pull request Apr 17, 2023
This PR is a updated revision of: #644

Removes copy of the project directory and instead only outputs
transliterated assemblies to the working directory. This avoids
potential problems when generating docs for packages that may have some
dependencies that are symlinks, such as those in a monorepo.

Fixes #390
@agdimech
Copy link
Contributor

This can be closed as #981 has been merged and released as of version ^7.2.1 :)

@MrArnoldPalmer
Copy link
Contributor Author

closing in favor of #981

@mrgrain mrgrain deleted the fix/incorrect-symlinks_390 branch April 9, 2024 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Issue with temporary folder and symlinks
4 participants