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

Add build and vars scripts to support building with sonarqube analysis #797

Closed
wants to merge 1 commit into from

Conversation

kyanha
Copy link
Contributor

@kyanha kyanha commented May 5, 2019

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the existing documentation
  • My changes generate no new warnings
  • I have updated the change log (Add/Change/Fix)
  • I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

  • Add a new script, based on build.bat, which will run the Sonar Scanner for MSBuild and the Sonar MSBuild Build Wrapper against the build. I understand that the project itself has a Travis Continuous Integration system set up for it; however, this is for the benefit of individual developers who want to run SonarQube/SonarCloud analysis against their own changes.
  • Modify the vswhere.exe command to reverse-sort the Visual Studio versions installed on the system. I currently have both VS2017 and VS2019 installed; I found out the hard way that this altered the default output of vswhere.exe. (I would do a simple version specification, but the version specification language available for vswhere breaks the FOR processing. See https://developercommunity.visualstudio.com/content/problem/556659/vswhereexe-documented-version-range-syntax-is-mang.html for the bug I filed on it.)
  • Reorder the build, so that the non-driver builds (both Release and Debug) are in one section and the driver builds (both Release and Debug) are in another. Also, so that x64 is built before Win32. This is to work around a SonarQube issue where only the first build is actually processed; this caused the sys/ directory to not show up in the analysis, because it isn't built for the non-OS-version-specific builds. Building x64 before Win32 is to make it so that the code build that's running on my system is the code that's analyzed, and can be undone if you'd prefer.

If documentation needs to be added to explain what needs to be changed to get it to work (basically, sonarvars.bat needs to be modified to plug in the correct values available from e.g. sonarcloud.io), I'll happily write it. Also, sonarvars.bat is a separate file so that it can perhaps be added to .gitignore so that secrets don't inadvertently get put in public repositories, while allowing the main logic of the script to be updated as appropriate.

)
)

%BUILD_WRAPPER% msbuild dokan.sln /p:Configuration="Win10 Release";Platform=x64 /t:Rebuild !CI_BUILD_ARG! || goto buildfailedsys
Copy link
Contributor

Choose a reason for hiding this comment

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

I have not cross-checked this, and my batch-skills became a bit rusty over the years: Can we somehow share some code with build.bat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually wrote this to do exactly the same as build.bat if it doesn't have the SonarQube variables set, which means "if sonarvars.bat doesn't exist". (I could rem out the SET commands in sonarvars.bat so that they wouldn't run, but it would make things more difficult if people didn't already understand that they needed to remove the REM statements. That's why I didn't offer a patch to overwrite build.bat.)

What I did was take build.bat and add a bunch of variable setting and checking. I also added %BUILD_WRAPPER% invocations before the 'msbuild' lines, because if you don't run it through the build wrapper then SonarScanner doesn't capture the output to figure out what it's supposed to analyze. (if %BUILD_WRAPPER% isn't set, then it expands to an empty string, so the msbuild command becomes the first thing on the line.)

Theoretically, it might be possible to share code with build.bat. It would require changing the order of the builds in build.bat (because of SonarScanner's ordering issue and the sys/ directory not being compiled in the non-version-specific msbuilds), and would require some fancy string matching in a FOR loop (so that only lines that start with "msbuild" would be invoked). It would also be lot more difficult (read: I personally can't figure out a way) to separate out the non-version-specific builds for general analysis and the version-specific builds for sys/ directory analysis. Input on how to accomplish it would be appreciated, since sys/ directory analysis is a concern.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @kyanha,
I agree with @Rondom it is a good idea you have but it share too much code with build.bat. We do not need to stick with batch code.
What could be done is to move build.bat and dokan_wix in scripts folder. Rewrite them in powershell to use helpers (build_helper.ps1) to avoid duplicate codes like vswhere / foreach build configuration and platform, .... Even create a coverity build script that use/download official tools instead of coveritypublisher #665
Like that appveyor script would be smaller and only set the needed env variables for coverity / sonar like you did with sonarvars.bat and call our build scripts.

@Liryna
Copy link
Member

Liryna commented Oct 14, 2019

Rewrote in ps here: bd8eb71

@Liryna
Copy link
Member

Liryna commented Oct 19, 2019

Done with 739a964

Thank you @kyanha for leading this idea 👍

@Liryna Liryna closed this Oct 19, 2019
@kyanha kyanha deleted the sonarbuild branch November 4, 2019 04:09
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.

None yet

3 participants