Skip to content

Conversation

@tustanivsky
Copy link
Collaborator

This PR addresses concerns regarding inconsistent line endings and BOM in some source files included in the plugin package. It also adds an automated check for these issues as part of the CI lint workflow (see #970 for additional details).

Related to #969

#skip-changelog

@jpnurmi
Copy link
Collaborator

jpnurmi commented Jul 2, 2025

Could it be easier done with .gitattributes?

*.h text eol=lf encoding=UTF-8
*.cpp text eol=lf encoding=UTF-8

And then git add --renormalize . once for the existing files.

@tustanivsky
Copy link
Collaborator Author

Could it be easier done with .gitattributes?

*.h text eol=lf encoding=UTF-8
*.cpp text eol=lf encoding=UTF-8

And then git add --renormalize . once for the existing files.

Yeah, adding .gitattributes will definitely help us avoid committing files with invalid line endings or BOM going forward. However keeping the CI checks still makes sense as an extra safety measure - for example, if we receive a PR from a user whose fork has a different configuration.

Comment on lines 15 to 24
- name: Check Line Endings
run: |
echo "Checking for consistent line endings (LF) in source files..."
files_with_crlf=$(find plugin-dev/Source/Sentry plugin-dev/Source/SentryEditor -name "*.h" -o -name "*.cpp" | xargs grep -l $'\r' 2>/dev/null || true)
if [ -n "$files_with_crlf" ]; then
echo "ERROR: Found files with CRLF line endings. Please convert all source files to use LF line endings."
echo "$files_with_crlf"
exit 1
fi
echo "✓ All source files have consistent LF line endings."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's a viable option. However, a separate check here seems to provide cleaner output compared to clang-format which tends to flood the logs with error: code should be clang-formatted messages for every line in a file with invalid line endings.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you could just push the clang-formatted code automatically

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done - clang-format now also checks for line endings and modifies sources in-place when needed. If any changes were made they will be pushed in the next step.

tustanivsky and others added 2 commits July 3, 2025 10:22
* Add code auto format

Change line ending for source file to test it out

* Fix script permissions

* Fix branch name

* Try different approach to checkout

* Try to checkout branch differently

* Test

* Test

* Format code

* Refactor workflow

* Test

* Clean up

* Format code

* Test

* Test

* Fix

* Test

* Format code

---------

Co-authored-by: Sentry Github Bot <[email protected]>
# Conflicts:
#	plugin-dev/Source/Sentry/Private/HAL/PlatformSentryAttachment.h
#	plugin-dev/Source/Sentry/Private/HAL/PlatformSentryScope.h
#	plugin-dev/Source/Sentry/Private/Microsoft/MicrosoftSentrySubsystem.cpp
#	plugin-dev/Source/Sentry/Private/Microsoft/MicrosoftSentrySubsystem.h
@tustanivsky tustanivsky requested a review from vaind July 3, 2025 11:10
Copy link
Collaborator

@vaind vaind left a comment

Choose a reason for hiding this comment

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

LFTM except for running on main

Comment on lines 36 to 39
- name: Commit Formatted Code
if: steps.clang-format.outputs.FORMATTING_CHANGED == 'true'
env:
GITHUB_BRANCH: ${{ github.ref_name }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't run on the main branch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point - added a corresponding check to if block here.

@tustanivsky tustanivsky merged commit d889090 into main Jul 3, 2025
29 checks passed
@tustanivsky tustanivsky deleted the ci/line-endings-and-bom branch July 3, 2025 12:22
@bruno-garcia
Copy link
Member

Do we need this in the console repos too?

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.

5 participants