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

Pull #12154: Use OS specific command and correct errorExtractingPattern #12154

Merged
merged 1 commit into from Sep 5, 2022

Conversation

Vyom-Yadav
Copy link
Member

@Vyom-Yadav Vyom-Yadav commented Sep 2, 2022

This PR contains:

1. Usage of OS specific command: (Exception without this modification in Windows OS) (Bug Fix)

Caught: java.io.IOException: Cannot run program "mvn": CreateProcess error=2, The system cannot find the file specified
java.io.IOException: Cannot run program "mvn": CreateProcess error=2, The system cannot find the file specified
        at error-prone-check.getErrorProneErrors(error-prone-check.groovy:87)
        at error-prone-check.checkErrorProneReport(error-prone-check.groovy:63)
        at error-prone-check.parseArgumentAndExecute(error-prone-check.groovy:41)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at error-prone-check.run(error-prone-check.groovy:19)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
Caused by: java.io.IOException: CreateProcess error=2, The system cannot find the file specified
        ... 10 more

2. Changed directory separator from / to [\\\\/] (Bug Fix)

3. Removed redundant escapes (\\]) (Enhancement)

@Vyom-Yadav
Copy link
Member Author

Works fine, tested locally on windows.

@Vyom-Yadav

This comment was marked as outdated.

@Vyom-Yadav Vyom-Yadav changed the title minor: Use mvn.cmd instead of mvn for windows OS and correct errorExtractingPattern minor: Use OS specefic command and correct errorExtractingPattern Sep 4, 2022
@Vyom-Yadav Vyom-Yadav changed the title minor: Use OS specefic command and correct errorExtractingPattern minor: Use OS specific command and correct errorExtractingPattern Sep 4, 2022
@Vyom-Yadav Vyom-Yadav force-pushed the windowsSupport-2 branch 2 times, most recently from 4367b05 to b9e4235 Compare September 4, 2022 18:17
@Vyom-Yadav Vyom-Yadav requested review from rnveach and nrmancuso and removed request for rnveach and nrmancuso September 4, 2022 18:27
@romani
Copy link
Member

romani commented Sep 4, 2022

please change commit to Pull #12154: ..... and explain kin PR description reason on why this update is required.
I am curious why were ok without in past and who will benefit from this update, and what problem had such people wihtout this update.

Copy link
Member

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

I am good as long as CI is passing and we have confirmation from @rnveach that this script works. I do not have access to a windows machine, and I do not think it is worth it to add to Windows CI just to prove it.

@nrmancuso nrmancuso assigned rnveach and unassigned nrmancuso Sep 4, 2022
@rnveach
Copy link
Member

rnveach commented Sep 4, 2022

Summary: TotalFiles=3 FilesWithViolations=1 P1=0 P2=0 P3=1

File: error-prone-check.groovy
    Violation: Rule=UnnecessarySemicolon P=3 Line=124 Msg=[Semi-colons as line endings can be removed safely] Src=[            .compile(".*[\\\\/](.*\\.java):\\[(\\d+).*\\[(\\w+)](.*)");]

[CodeNarc (http://www.codenarc.org/) v1.4]
CodeNarc completed: (p1=0; p2=0; p3=1) 5768ms

Copy link
Member

@rnveach rnveach left a comment

Choose a reason for hiding this comment

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

CI failure.

@Vyom-Yadav Vyom-Yadav changed the title minor: Use OS specific command and correct errorExtractingPattern Pull #12154: Use OS specific command and correct errorExtractingPattern Sep 5, 2022
@Vyom-Yadav
Copy link
Member Author

CI failure.

Fixed

@romani
Copy link
Member

romani commented Sep 5, 2022

Usage of OS specific command:

Please share a command and what OS you use.
I still do not understand what problem we have.we never had such problem. In what condition it happens?

@Vyom-Yadav
Copy link
Member Author

Usage of OS specific command:

Please share a command and what OS you use. I still do not understand what problem we have.we never had such problem. In what condition it happens?

Only this script uses maven. We never had this problem because the contribution repo is handling it properly. See https://github.com/checkstyle/contribution/blob/1b3e33600898a849791dc88c90df2bb3f80400e7/checkstyle-tester/diff.groovy#L669-L689

You can normally execute mvn clean verify in windows, but when you use it like command.execute() then it throws and exception.

@Vyom-Yadav
Copy link
Member Author

@romani
Copy link
Member

romani commented Sep 5, 2022

Ok it is win os, for new script that we created recently.

@rnveach rnveach merged commit 6fee303 into checkstyle:master Sep 5, 2022
@romani romani added this to the 10.3.4 milestone Sep 5, 2022
rnveach pushed a commit to rnveach/checkstyle that referenced this pull request Sep 20, 2022
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.

None yet

4 participants