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

Enforce ASCII and whitespace conventions #229

Merged
merged 14 commits into from
Oct 30, 2019

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented Oct 29, 2019

Description

Fixes #141. This doesn't attempt to enforce 120 columns.

I've performed extensive manual testing of validate.cpp (in particular, for every validation failure). Initially, I didn't test the CMake and Azure DevOps changes at all. However, they've successfully built and passed in this PR, and intentionally failed in #230, which I believe is sufficient testing.

This also fixes accumulated issues:

  • Change tabs to spaces.

    • .gitmodules
  • Remove UTF-8 BOMs.

    • CMakeSettings.json
    • tools/CMakeSettings.json
  • Change LF to CRLF.

    • .gitmodules
    • azure-devops/install_msvc_preview.ps1
    • azure-devops/run_build.yml
    • azure-pipelines.yml
    • docs/ISSUE_TEMPLATE.md
    • vcpkg_windows.txt

Checklist

If you're unsure about a box, leave it unchecked. A maintainer will help you.

  • I understand README.md. I also understand that acceptance of
    community PRs will be delayed until the test and CI systems are online.
  • Identifiers in product code changes are properly _Ugly as per
    https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
  • The STL builds successfully and all tests have passed (must be manually
    verified by an STL maintainer before CI is online, leave this unchecked for
    initial submission).
  • These changes introduce no known ABI breaks (adding members, renaming
    members, adding virtual functions, changing whether a type is an aggregate
    or trivially copyable, etc.).
  • These changes were written from scratch using only this repository and
    the C++ Working Draft as a reference (and any other cited standards).
    If they were derived from a project that's already listed in NOTICE.txt,
    that's fine, but please mention it. If they were derived from any other
    project (including Boost and libc++, which are not yet listed in
    NOTICE.txt), you must mention it here, so we can determine whether the
    license is compatible and what else needs to be done.

@StephanTLavavej
Copy link
Member Author

The CMake and Azure DevOps changes appear to be successful (although I haven't tested them with an intentional validation failure yet).

  1. validate.exe built successfully:
##[section]Starting: Build Support Tools
[...]
Building with CMake in build directory 'C:\agent\_work\1\a/tools' ...
[command]"C:\Program Files\CMake\bin\cmake.exe" --build .
[1/6] Building CXX object validate\CMakeFiles\validate.dir\validate.cpp.obj
[2/6] Building CXX object jobify\CMakeFiles\jobify.dir\jobify.cpp.obj
[3/6] Building CXX object parallelize\CMakeFiles\parallelize.dir\parallelize.cpp.obj
[4/6] Linking CXX executable validate\validate.exe
[5/6] Linking CXX executable jobify\jobify.exe
[6/6] Linking CXX executable parallelize\parallelize.exe
##[section]Finishing: Build Support Tools
  1. validate-files.cmd ran successfully:
##[section]Starting: Validate Files
==============================================================================
Task         : Batch script
Description  : Run a Windows command or batch script and optionally allow it to change the environment
Version      : 1.1.7
Author       : Microsoft Corporation
Help         : https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/batch-script
==============================================================================
##[command]C:\agent\_work\1\s\azure-devops\validate-files.cmd C:\agent\_work\1\a/tools/validate/validate.exe

C:\agent\_work\1\s>"C:\agent\_work\1\a/tools/validate/validate.exe"
If your build fails here, you need to fix the listed issues.
##[section]Finishing: Validate Files
  1. The wrapping in enforce-clang-format.cmd didn't affect its operation:
##[section]Starting: Enforce clang-format
==============================================================================
Task         : Batch script
Description  : Run a Windows command or batch script and optionally allow it to change the environment
Version      : 1.1.7
Author       : Microsoft Corporation
Help         : https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/batch-script
==============================================================================
##[command]C:\agent\_work\1\s\azure-devops\enforce-clang-format.cmd C:\agent\_work\1\a/tools/parallelize/parallelize.exe

C:\agent\_work\1\s>"C:\agent\_work\1\a/tools/parallelize/parallelize.exe" "clang-format.exe -style=file -i" stl/inc stl/inc/cvt stl/inc/experimental stl/src tools/inc tools/jobify/jobify.cpp tools/parallelize/parallelize.cpp tools/validate/validate.cpp 
1 scheduled; 0 completed; 1 running
2 scheduled; 0 completed; 2 running
[...]
466 scheduled; 465 completed; 1 running
466 scheduled; 466 completed; 0 running
466 commands ran, returned 0, and produced no output.
If your build fails here, you need to format the following files with
clang-format version 9.0.0 (tags/RELEASE_900/final)
##[section]Finishing: Enforce clang-format

@StephanTLavavej StephanTLavavej mentioned this pull request Oct 29, 2019
5 tasks
@StephanTLavavej
Copy link
Member Author

And now I've tested intentional validation failure with #230 (by creating a separate branch with the "fix accumulated issues" commits dropped). That fails in what I think is a very nice way, with the Azure Pipelines UI describing what files are affected and how.

tools/validate/validate.cpp Show resolved Hide resolved
tools/validate/validate.cpp Show resolved Hide resolved
.gitmodules Outdated Show resolved Hide resolved
azure-devops/validate-files.cmd Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
tools/validate/validate.cpp Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member Author

@BillyONeal , should .gitmodules be allowed to remain LF, or be changed to CRLF? If the former, I'll need to add another allow list, and revert that change.

@BillyONeal
Copy link
Member

@BillyONeal , should .gitmodules be allowed to remain LF, or be changed to CRLF? If the former, I'll need to add another allow list, and revert that change.

The one in msvc is \t and CRLF, so I think the CRLF change is fine. (The PR that added most of the LF files fixed here also added that .gitmodules)

@StephanTLavavej
Copy link
Member Author

StephanTLavavej commented Oct 30, 2019

continueOnError doesn't do what we want. Checks still pass, saying "succeeded with issues".

I've discovered that eliminating the batch file had highly undesirable effects; I'm going to restore it.

@StephanTLavavej
Copy link
Member Author

I've force-pushed #230 accordingly, showing what happens when validation fails.

@StephanTLavavej StephanTLavavej merged commit 9c1c63c into microsoft:master Oct 30, 2019
@StephanTLavavej StephanTLavavej deleted the validate branch October 30, 2019 23:22
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.

GitHub: PR system should enforce ASCII and whitespace conventions
4 participants