fix: Fail-fast if the path to npm cache contains whitespaces #11517

mshabarov · 2021-08-04T07:31:46Z

Description

Checks that the npm version accepts whitespaces in user home path and explains how to fix that otherwise.

Fixes #10084

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed self-review and corrected misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

Fixes #10084

sonatype-lift · 2021-08-04T07:55:48Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/FrontendStubs.java

+
+        private ToolStubInfo(boolean stubbed, String version) {
+            this.stubbed = stubbed;
+            assert !stubbed || version != null && !version


OperatorPrecedence: Use grouping parenthesis to make the operator precedence explicit
(at-me in a reply with help or ignore)

sonatype-lift · 2021-08-04T07:55:50Z

flow-test-generic/src/main/java/com/vaadin/flow/testutil/FrontendStubs.java

            File binDir = new File(baseDir, "node/node_modules/npm/bin");
            FileUtils.forceMkdir(binDir);
-            String stub = "process.argv.includes('--version') && console.log('6.14.10');";
+            String stub = String.format("process.argv.includes('--version') && "
+                    + "console.log(%s);", stubNpm.getVersion());
            FileUtils.writeStringToFile(new File(binDir, "npm-cli.js"), stub,
                    StandardCharsets.UTF_8);
            FileUtils.writeStringToFile(new File(binDir, "npx-cli.js"), stub,
                    StandardCharsets.UTF_8);
        }
        boolean isWindows = System.getProperty("os.name").startsWith("Windows");


NULL_DEREFERENCE: object returned by getProperty("os.name") could be null and is dereferenced at line 76.
(at-me in a reply with help or ignore)

…r-home

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendTools.java

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java

flow-server/src/test/java/com/vaadin/flow/server/frontend/FrontendToolsTest.java

flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java

sonatype-lift · 2021-08-05T13:00:52Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendTools.java

+            npmCacheCommand.add("--global");
+            String output = FrontendUtils.executeCommand(npmCacheCommand);
+            output = parseCommandOutput(output);
+            if (output.isEmpty()) {


NULL_DEREFERENCE: object output last assigned on line 524 could be null and is dereferenced at line 525.
(at-me in a reply with help or ignore)

This reverts commit 4a497d3

This reverts commit 4005db3

mshabarov · 2021-08-05T19:17:20Z

TaskRunPnpmInstallTest and FrontendToolsTest tests successfully run on Windows agent and show the message as expected.

…r-home

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendTools.java

denis-anisimov · 2021-08-06T12:06:41Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendTools.java

@@ -724,4 +799,11 @@ private String doGetNodeExecutable() {
            return getNodeExecutable();
        }
    }
+
+    private String parseCommandOutput(String output) {


parse ?
What kind of parsing?
This is rather a conversation or just a "normalization"

Yes, it's not a parsing, renamed to removeLineBreaks.

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java

flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskRunNpmInstall.java

flow-server/src/test/java/com/vaadin/flow/server/frontend/TaskRunPnpmInstallTest.java

vaadin-bot · 2021-08-06T16:15:42Z

SonarQube analysis reported 15 issues

6 major
8 minor
1 info

Top 10 issues

FrontendTools.java#L525: A "NullPointerException" could be thrown; "output" is nullable here.
TaskRunNpmInstall.java#L92: Remove this unused "classFinder" private field.
TaskRunNpmInstall.java#L294: Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed.
FrontendStubs.java#L82: Exceptional return value of java.io.File.createNewFile() ignored in com.vaadin.flow.testutil.FrontendStubs.createStubNode(FrontendStubs$ToolStubInfo, FrontendStubs$ToolStubInfo, String)
FrontendStubs.java#L89: This block of commented-out lines of code should be removed.
FrontendStubs.java#L148: Exceptional return value of java.io.File.createNewFile() ignored in com.vaadin.flow.testutil.FrontendStubs.createStubWebpackServer(String, int, String, boolean)
FrontendTools.java#L517: Remove the declaration of thrown exception 'java.lang.IllegalStateException' which is a runtime exception.
FrontendUtils.java#L482: Remove this use of "closeQuietly"; it is deprecated.
FrontendUtils.java#L536: Remove this use of "closeQuietly"; it is deprecated.
TaskRunNpmInstall.java#L527: Remove the declaration of thrown exception 'java.lang.IllegalStateException' which is a runtime exception.

vaadin-bot · 2021-08-09T06:04:13Z

Hi @mshabarov and @denis-anisimov, when i performed cherry-pick to this commit to 2.7, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick ec02a2a
error: could not apply ec02a2a... fix: Fail-fast if the path to npm cache contains whitespaces (#11517)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

vaadin-bot · 2021-08-30T08:50:00Z

This ticket/PR has been released with platform 22.0.0.alpha2 and is also targeting the upcoming stable 22.0.0 version.

mshabarov added 2 commits August 4, 2021 10:29

fix: Fail with a message if user home contains whitespaces

8fab39a

Fixes #10084

Fix formatter annotation

065d1fc

sonatype-lift bot reviewed Aug 4, 2021

View reviewed changes

mshabarov added 2 commits August 4, 2021 12:08

Fix stub script and address sonar issues

612727b

Merge remote-tracking branch 'origin/master' into fix/whitespaces-use…

f6ffe12

…r-home

denis-anisimov suggested changes Aug 4, 2021

View reviewed changes

mshabarov added 3 commits August 5, 2021 09:05

Refactored and reworked to check npm cache directory

d567703

Fix formatting

200ec58

Fix build tool stubs

b9950ee

vaadin-bot added the +0.1.0 label Aug 5, 2021

mshabarov added 2 commits August 5, 2021 12:53

Adapt tests for windows

5879cd4

Adapt tests for windows

994e424

sonatype-lift bot reviewed Aug 5, 2021

View reviewed changes

mshabarov added 7 commits August 5, 2021 16:05

Add temporary logging

4005db3

Add temporary logging

4a497d3

Escape backslashes in script

aeb82db

Remove formatting

47751f0

Revert "Add temporary logging"

1104d77

This reverts commit 4a497d3

Revert "Add temporary logging"

b59779f

This reverts commit 4005db3

Check the first line of message

0e8ba14

vaadin deleted a comment from vaadin-bot Aug 5, 2021

Merge remote-tracking branch 'origin/master' into fix/whitespaces-use…

a45bfc7

…r-home

mshabarov changed the title ~~fix: Fail with a message if user home contains whitespaces~~ fix: Fail-fast if the path to npm cache contains whitespaces Aug 6, 2021

Merge remote-tracking branch 'origin/master' into fix/whitespaces-use…

14745f3

…r-home

mshabarov added target/2.7 labels Aug 6, 2021

mshabarov requested a review from denis-anisimov August 6, 2021 10:08

mshabarov removed target/2.6 labels Aug 6, 2021

denis-anisimov suggested changes Aug 6, 2021

View reviewed changes

mshabarov added 3 commits August 6, 2021 18:27

Address review comments

d0bd10b

Catch process execution exceptions immediately

4377418

Remove leftovers

39749aa

mshabarov requested a review from denis-anisimov August 9, 2021 05:57

denis-anisimov approved these changes Aug 9, 2021

View reviewed changes

denis-anisimov merged commit ec02a2a into master Aug 9, 2021

denis-anisimov deleted the fix/whitespaces-user-home branch August 9, 2021 06:00

vaadin-bot added the need to pick manually 2.7 label Aug 9, 2021

vaadin-bot added prerelease version for platform 22.0.0 Released with Platform 22.0.0.alpha2 labels Aug 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fail-fast if the path to npm cache contains whitespaces #11517

fix: Fail-fast if the path to npm cache contains whitespaces #11517

mshabarov commented Aug 4, 2021 •

edited

Loading

sonatype-lift bot Aug 4, 2021

sonatype-lift bot Aug 4, 2021

sonatype-lift bot Aug 5, 2021

mshabarov commented Aug 5, 2021 •

edited

Loading

denis-anisimov Aug 6, 2021

mshabarov Aug 6, 2021

vaadin-bot commented Aug 6, 2021

vaadin-bot commented Aug 9, 2021

vaadin-bot commented Aug 30, 2021

fix: Fail-fast if the path to npm cache contains whitespaces #11517

fix: Fail-fast if the path to npm cache contains whitespaces #11517

Conversation

mshabarov commented Aug 4, 2021 • edited Loading

Description

Type of change

Checklist

Additional for Feature type of change

sonatype-lift bot Aug 4, 2021

Choose a reason for hiding this comment

sonatype-lift bot Aug 4, 2021

Choose a reason for hiding this comment

sonatype-lift bot Aug 5, 2021

Choose a reason for hiding this comment

mshabarov commented Aug 5, 2021 • edited Loading

denis-anisimov Aug 6, 2021

Choose a reason for hiding this comment

mshabarov Aug 6, 2021

Choose a reason for hiding this comment

vaadin-bot commented Aug 6, 2021

Top 10 issues

vaadin-bot commented Aug 9, 2021

vaadin-bot commented Aug 30, 2021

mshabarov commented Aug 4, 2021 •

edited

Loading

Additional for `Feature` type of change

mshabarov commented Aug 5, 2021 •

edited

Loading