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: Add option to not modify readonly files during copy, add logging of time #19894

Merged
merged 7 commits into from
Sep 6, 2024

Conversation

TatuLund
Copy link
Contributor

@TatuLund TatuLund commented Sep 5, 2024

Allows to skip setting the writable flag on copied files, by providing the vaadin.frontend.disableWritableFlagCheckOnCopy system property.
This may improve performance in certain scenarios with Windows OS.

Fixes #19866

…g of time

This may improve performance in certain scenarios with Windows OS.
@thomasharre
Copy link

@TatuLund you have changed the default behaviour, I only wanted a possibility to turn it of.
I suggest changing:
PREVENT_READONLY_FILES = true;

Copy link

github-actions bot commented Sep 5, 2024

Test Results

1 135 files  ± 0  1 135 suites  ±0   1h 25m 54s ⏱️ - 8m 41s
7 392 tests ± 0  7 342 ✅ ± 0  50 💤 ±0  0 ❌ ±0 
7 705 runs   - 57  7 645 ✅  - 57  60 💤 ±0  0 ❌ ±0 

Results for commit a30a754. ± Comparison against base commit 4b91372.

♻️ This comment has been updated with latest results.

@TatuLund
Copy link
Contributor Author

TatuLund commented Sep 5, 2024

@thomasharre Thanks for spotting that. I was testing this locally changing the value back and forth, it was wrong value by accident.

@mcollovati
Copy link
Collaborator

What's the idea behind the usage of the static field? Is the developer supposed to change it in the application code?
If so, this will work for development mode, but will not help when the task is executed by Maven or Gradle.
Could a system property be a better option?

@TatuLund
Copy link
Contributor Author

TatuLund commented Sep 5, 2024

Could a system property be a better option?

Yes it would.

@thomasharre
Copy link

We are only experiencing problems in certain developer environements.
Therefor the static field is the most pragmatic solution for us.

A system property would also work, but the current solution with public static field would be a fast way to help us.

@mcollovati
Copy link
Collaborator

I still would prefer a system property, instead of the static field. Something like vaadin.frontend.disableReadonlyCheckOnCopy (feel free to suggest a better name), to make explicit that we are disabling an operation that was added to fix application failures (for example #12711)

@thomasharre
Copy link

thomasharre commented Sep 5, 2024

A system property would also work fine for me.

So the code in copyLocalResources would be something like?
boolean performReadonlyCheck = !Boolean.valueOf(System.getProperty("vaadin.frontend.disableReadonlyCheckOnCopy", "false")); if (performReadonlyCheck){ ... }

@TatuLund would that suit you? If yes, could you please commit that?

@thomasharre
Copy link

@TatuLund the member 'performReadonlyCheck' should not be static

@TatuLund
Copy link
Contributor Author

TatuLund commented Sep 5, 2024

the member 'performReadonlyCheck' should not be static

@thomasharre No, it must be as the method where it is used is static

@thomasharre
Copy link

thomasharre commented Sep 5, 2024

@TatuLund
setting a static member in the constructor can't be right
then i would delete the member and introduce a new static method:

private static boolean performReadonlyCheck(){ return !Boolean.valueOf(System.getProperty("vaadin.frontend.disableReadonlyCheckOnCopy", "false")); }

@TatuLund
Copy link
Contributor Author

TatuLund commented Sep 5, 2024

@thomasharre Both work, but you are right it is cleaner version. Funny that Sonar did not complain about it.

@thomasharre
Copy link

@TatuLund thanks

@mcollovati could you take another look and descide if this can be merged?

mcollovati
mcollovati previously approved these changes Sep 5, 2024
Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

Just a comment about naming, other than that LGTM

@mcollovati mcollovati enabled auto-merge (squash) September 6, 2024 09:07
@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Sep 6, 2024
Copy link

sonarcloud bot commented Sep 6, 2024

@mcollovati mcollovati merged commit 18761fd into main Sep 6, 2024
26 of 42 checks passed
@mcollovati mcollovati deleted the fix19866 branch September 6, 2024 09:22
vaadin-bot pushed a commit that referenced this pull request Sep 6, 2024
…g of time (#19894)

Allows to skip setting writable flag on copied files by providing the vaadin.frontend.disableWritableFlagCheckOnCopy system property.
This may improve performance in certain scenarios with Windows OS.

Fixes #19866
vaadin-bot added a commit that referenced this pull request Sep 6, 2024
…g of time (#19894) (#19906)

Allows to skip setting writable flag on copied files by providing the vaadin.frontend.disableWritableFlagCheckOnCopy system property.
This may improve performance in certain scenarios with Windows OS.

Fixes #19866

Co-authored-by: Tatu Lund <[email protected]>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.alpha16 and is also targeting the upcoming stable 24.5.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TaskCopyLocalFrontendFiles#copyLocalResources(File, File, String... ) takes a long time using Windows-OS
4 participants