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 only single rollup-plugin-copy() to avoid race condition #843

Merged
merged 6 commits into from Oct 12, 2022

Conversation

JeffersGlass
Copy link
Member

#839 improved rollup logic - it always copies build folder to examples/build, regardless of whether this is a production build or not. However, it seems that rollup-plugin-copy doesn't behave nicely when used multiple times in a single rollup config. Possibly a race condition or data corruption? Errors include EONENT: file not found 'build/index.html', EBUSY: resource busy or locked on Linux and Windows build systems.

This PR moves that logic outside of the exported rollup config, to avoid the error.

@@ -47,14 +61,7 @@ export default {
}),
// This will make sure that examples will always get the latest build folder
!production && copy({
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are managing file copy_targets, let's remove the !production on line 63 limiting this to not-production

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed and removed.

@tedpatrick
Copy link
Contributor

I think we need to revisit this one.

  1. Definition of production is polluted by https://github.com/pyscript/pyscript/blob/main/pyscriptjs/rollup.config.js#L12
  2. Copying the directory listing (public/index.html) needs to run on all builds, not just !production
  3. Some duplicate rollup commands to remove https://github.com/pyscript/pyscript/blob/main/pyscriptjs/rollup.config.js#L59-L60

@tedpatrick
Copy link
Contributor

@JeffersGlass
Copy link
Member Author

Thanks @tedpatrick ! I've adjusted the file per your thoughts, removing the duplicate commands and cleaning up the definition of copy_targets at the top there.

@tedpatrick
Copy link
Contributor

tedpatrick commented Oct 12, 2022

It looks like the serve command is called in GitHub Actions during the build and it stops and listens
Screen Shot 2022-10-11 at 9 11 41 PM

@tedpatrick
Copy link
Contributor

tedpatrick commented Oct 12, 2022

I think it is this line defining production

const production = !process.env.ROLLUP_WATCH || (process.env.NODE_ENV === "production");

@JeffersGlass
Copy link
Member Author

I think it is this line defining production

const production = !process.env.ROLLUP_WATCH || (process.env.NODE_ENV === "production");

Thanks for stopping the action - good catch! And interesting - that line has been in that form for ages... but in the commit that triggered that broken build 06fe46a, I had changed it to:

-   const production = !process.env.ROLLUP_WATCH || (process.env.NODE_ENV === "production");
+   const production = (process.env.NODE_ENV === "production");

Maybe that ROLLUP_WATCH flag is doing more than I thought... let me push a commit adding that flag back in and we'll see what happens in CI...

Ted Patrick and others added 4 commits October 12, 2022 11:10
* Upgrade and Pin Dependencies

* Removal all unused deps

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* lock

* remove svelte-fa reference from the svg style.

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
updates:
- [github.com/psf/black: 22.8.0 → 22.10.0](psf/black@22.8.0...22.10.0)
- [github.com/asottile/pyupgrade: v2.38.2 → v3.1.0](asottile/pyupgrade@v2.38.2...v3.1.0)
- [github.com/pre-commit/mirrors-eslint: v8.23.1 → v8.25.0](pre-commit/mirrors-eslint@v8.23.1...v8.25.0)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Hide py-config element

* display:none for all web components

* docs

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add style tests for raw html and pyscript enabled html

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* test naming

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@tedpatrick
Copy link
Contributor

Rerunning actions. It looked to fail in an integration test. But the fact that it reaches integration is positive!

@tedpatrick
Copy link
Contributor

Yes I agree, the !process.env.ROLLUP_WATCH is confusing

@tedpatrick
Copy link
Contributor

@JeffersGlass Merge when ready.

Merge policy: You build it, you merge it!

@JeffersGlass JeffersGlass merged commit b184c92 into pyscript:main Oct 12, 2022
@JeffersGlass JeffersGlass deleted the copy_targets branch October 12, 2022 17:05
@tedpatrick
Copy link
Contributor

Great work @JeffersGlass, Thanks.

JeffersGlass added a commit to JeffersGlass/pyscript that referenced this pull request Oct 17, 2022
…t#843)

* Use only single copy() plugin to avoid race condition

* clean production def, better copy_target structure

* Restore '!process.env.ROLLUP_WATCH to production definition

Co-authored-by: Ted Patrick <tpatrick@anaconda.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants