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

Many useless trailing blanks in source code #15114

Open
guillaumelambert opened this issue May 17, 2023 · 2 comments
Open

Many useless trailing blanks in source code #15114

guillaumelambert opened this issue May 17, 2023 · 2 comments
Assignees
Labels
MSFT Triaged this issue has been triaged

Comments

@guillaumelambert
Copy link
Contributor

Trailing blanks are generally and strongly dis-advised by most programming languages best practices.
It is commonly admitted that they complicate the code maintenance.
Though, many can be found in the sonic-buidimage repo source code (about 17400 lines).
Also no dedicated linter to address this issue can be found in the SONiC Azure CI.

guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 19, 2023
- run pre-commit tox profile to trim all trailing blanks
- use several commits with a per-folder based strategy
  to ease their merge

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 19, 2023
- run pre-commit tox profile to trim all trailing blanks
- use several commits with a per-folder based strategy
  to ease their merge

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 19, 2023
- run pre-commit tox profile to trim all trailing blanks
- use several commits with a per-folder based strategy
  to ease their merge

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 19, 2023
… blanks

- run pre-commit tox profile to trim all trailing blanks
- use several commits with a per-folder based strategy
  to ease their merge

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 19, 2023
- Run pre-commit tox profile to trim all trailing blanks
- Use several commits with a per-folder based strategy
  to ease their merge

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 24, 2023
- add a pre-commit yaml configuration file with a basic profile to
  remove trailing blanks
- add a rule to exclude from its scope
  - patch files (generated from git diff)
    Trailing blanks are part of their syntax.
  - .conf files in the src folder
    Their trailing blanks removal make some tests fail during build.
  - jinja templates in docker folder
    Though trailing blank should not be part of the jinja2 syntax,
    these files use a known hack that requires trailing blanks.
    Their removal makes build fail.
- create a tox configuration to run/install/uninstall/autoupdate
  pre-commit in a python venv
- create a tox profile for the CI
  to run pre-commit only on files modified since this commit
- add a shell script to ease running pre-commit inside various CI

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 24, 2023
Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 24, 2023
Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
lguohan pushed a commit that referenced this issue May 24, 2023
… blanks (#15161)

- run pre-commit tox profile to trim all trailing blanks
- use several commits with a per-folder based strategy
  to ease their merge

Issue #15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
lguohan pushed a commit that referenced this issue May 24, 2023
- Run pre-commit tox profile to trim all trailing blanks
- Use several commits with a per-folder based strategy
  to ease their merge

Issue #15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 24, 2023
- add a pre-commit yaml configuration file with a basic profile to
  remove trailing blanks
- add a rule to exclude from its scope
  - patch files (generated from git diff)
    Trailing blanks are part of their syntax.
  - .conf files in the src folder
    Their trailing blanks removal make some tests fail during build.
  - jinja templates in docker folder
    Though trailing blank should not be part of the jinja2 syntax,
    these files use a known hack that requires trailing blanks.
    Their removal makes build fail.
- create a tox configuration to run/install/uninstall/autoupdate
  pre-commit in a python venv
- create a tox profile for the CI
  to run pre-commit only on files modified since this commit
- add a shell script to ease running pre-commit inside various CI

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 24, 2023
In azure pipelines configuration
- add a linter stage
- add to the linter stage a tox job w/ 2 steps to run tox

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>

fix
guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 24, 2023
Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
@liushilongbuaa
Copy link
Contributor

liushilongbuaa commented May 25, 2023

grep -Eo " *$" ./ -r --exclude-dir .git | awk -F: '{print$1}' | xargs -i sudo sed -i "s/ *$//" {}
How about bash command to remove tail spaces?
I tried locally and got 17477 lines added, 17481 lines removed.
git diff --numstat | awk '{s+=$1} END {print s}'
But removing all tail spaces is a big PR. It is hard to review.
If we add it to CI pipeline. We need to push commits to original PR. There maybe permission issue.

guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 25, 2023
…g blanks

- run pre-commit tox profile to trim all trailing blanks
- use several commits with a per-folder based strategy
  to ease their merge

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
@guillaumelambert
Copy link
Contributor Author

grep -Eo " *$" ./ -r --exclude-dir .git | awk -F: '{print$1}' | xargs -i sudo sed -i "s/ *$//" {} How about bash command to remove tail spaces? I tried locally and got 17477 lines added, 17481 lines removed. git diff --numstat | awk '{s+=$1} END {print s}' But removing all tail spaces is a big PR. It is hard to review. If we add it to CI pipeline. We need to push commits to original PR. There maybe permission issue.

Thanks for your feedback. Such shell scripts commands do work.
Though, as stated in the commit Orange-OpenSource@1aebc16 in one of the related PRs, trailing blanks removal of some .conf files in the src folder made some tests fail during build phase. And jinja templates in docker folder use a known hack that requires trailing blanks... So we will have to deal with some exceptions at least temporarily. This means incorporating more commands and potentially complex regex in such a shell script.
I proposed in #15115 a different solution based on tox & pre-commit, which is IMHO more flexible and maintainable in a CI. It embeds a tox profile dedicated to the CI that allows to check only files modified since the introduction of this PR.

I do agree that trailing blanks complete trimming would be a big PR.
cf #15115 (comment)
I already proposed and split it in several pieces to ease its review and merge. Hope this helps. I noticed some have been already merged.

I also understand from your comment that sudo permissions are required to clean some files. But I haven't caught where.
Please can you be more precise about it. Is it all over the code or just a specific folder ? It may explain the difference of modified lines you observed. Thx in advance.

guillaumelambert added a commit to Orange-OpenSource/sonic-buildimage that referenced this issue May 25, 2023
- run pre-commit tox profile to trim all trailing blanks
- use several commits with a per-folder based strategy
  to ease their merge

Issue sonic-net#15114

Signed-off-by: Guillaume Lambert <guillaume.lambert@orange.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MSFT Triaged this issue has been triaged
Projects
None yet
Development

No branches or pull requests

3 participants