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

Use random temp directory for progress estimator (fix concurrent builds) #10

Merged
merged 3 commits into from
May 14, 2021

Conversation

piter2k1
Copy link
Contributor

@piter2k1 piter2k1 commented May 12, 2021

I have been using rollpkg to build packages in monorepo by lerna (https://github.com/lerna/lerna). I have one instance of rollpkg in root monorepo directory. lerna builds 5-6 packages concurrently and sometimes one of builds ends with progress-estimator error. It uses the same cache folder for every build process, and sometimes it creates file conflict.

I created this update to resolve this issue by using system tmp directory with random sub-directory for every new rollpkg build. I've been testing this for a month in my local monorepo and proposed solution works without any problems.

image

@rafgraph
Copy link
Owner

@piter2k1 thanks for the PR. Supporting monorepos with concurrent builds is a good feature to add. There are two things that I am not sure about:

  1. progress-estimator estimates the progress based on how long the previous run(s) took. Without the previous data it just shows a spinner without a progress bar. Is it possible to identify the name of the current package being built and use that for the directory name where progress-estimator data is stored? Instead of fs.mkdtemp(...) which creates a new directory name each time.
  2. The current use of __dirname instead of os.tmpdir() means that progress-estimator data is stored in node_modules/rollpkg instead of at the os level. Curious if there is there a reason for the change? Would using fs.mkdtemp(join(__dirname, 'progress-estimator-')) work? (or use the package name instead of fs.mkdtemp(...) as per 1 above).

@rafgraph
Copy link
Owner

Also, is the monorepo you're using public? Or would you be able to create an example monorepo that uses rollpkg? This would be helpful to me (for testing) and other Rollpkg users (as an example to follow). Thanks.

@piter2k1
Copy link
Contributor Author

@rafgraph You may be right. I will try some other implementations. I made this code as hot fix for our monorepo, so maybe I can fix this issue with simpler solution.

Also, i will try to make repository with monorepo for testing it, but I need more time for that.

@piter2k1
Copy link
Contributor Author

piter2k1 commented May 14, 2021

I made some research in other libraries and I think I found a good solution.

I used external library find-cache-dir to find node_modules/.cache directory and to use it to store progress estimator cache files (and other in future, if you want). This library is used by terser-webpack-plugin and rollup-plugin-typescript2 for the same reason, so it is available in current rollpkg dependencies.

Now, progress-estimator cache is saved in local node_modules/.cache/rollpkg/progress-estimator directory for each package. This is also a big advantage for building monorepos, because cache is stored in each monorepo package separately. rollup-plugin-typescript2 do exactly the same during lerna build.

image

I tested this on local monorepo (mac os) and on our company server by Jenkins build(docker centos), everythink was fine.

And one more, when working with monorepo, we can run lerna clean to remove node_modules from all packages with .cache files.

So, what is changed?

  • Cache directory is moved from node_modules/rollpkg/dist/.progress-estimator to node_modules/.cache/rollpkg/progress-estimator. I think this is generally better way to store cache.
  • For monorepo, local package node_modules dir is used, not project root dir. For separate files during build (rollup-plugin-typescript2 do the same)

More about node_modules/.cache pattern: https://github.com/avajs/find-cache-dir

Percent progress estimator naturally also works fine 😄

Jenkins log fragment:
jenkins___CUE_pr-test-bb_tvn-components___PR-409____2

@rafgraph
Copy link
Owner

@piter2k1 thanks for updating the PR. I really like your solution! I agree that node_modules/.cache/rollpkg/progress-estimator is a better place to store cached data.

I'm going to merge this and release a new patch version.

It would be much appreciated if you could create an example monorepo using Rollpkg. I will link to it from the readme for others to use an example, as well as it will be helpful for smoke testing to prevent breaking monorepo support in the future. Thanks!

@rafgraph rafgraph merged commit c524afa into rafgraph:main May 14, 2021
@rafgraph
Copy link
Owner

Released v0.5.7 🎉🎉

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.

2 participants