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

FIx violations and enable --warn=all #30854

Merged
merged 10 commits into from
Jan 11, 2024
Merged

Conversation

bongbui321
Copy link
Contributor

Description [](A description of the refactor, including the goals it accomplishes.)
For #29500, Fix violations and enable warn=all

.github/workflows/selfdrive_tests.yaml Outdated Show resolved Hide resolved
SConstruct Outdated Show resolved Hide resolved
Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

Is this really the source of the remaining warnings? This is a pretty core part of SCons, and we shouldn't be doing this scanning ourselves.

Is there a cleaner way to properly fix this? Maybe we can filter these warnings instead?

@adeebshihadeh adeebshihadeh marked this pull request as draft January 8, 2024 00:09
@bongbui321
Copy link
Contributor Author

bongbui321 commented Jan 9, 2024

@adeebshihadeh I looked into it and I agree that we shouldn't replace SCons Scanner. I replaced the default ClassicCPP SCanner with CConditional Scanner which resolves (#ifdef, etc) statements and not include non-existing file.

However, I made some modification to the Source Code and ignore the C standard files, poetry virtualenv (.venv folder) and apt packages since we never explicitly modify these files. Also Having these files being scanned slows down the compilation time significantly since there are alot.

The test cases are failing because of the issue here. When that PR is merged, it should compile normally

Also I talked to scons maintainer, and he agrees that missing system header files warning might be change in the future. So I think this temporal implementation right now works fine.

@bongbui321 bongbui321 marked this pull request as ready for review January 9, 2024 16:13
@adeebshihadeh
Copy link
Contributor

Works only with the assumption that every important implicit dependency is within the repo

Unfortunately, we rely on that behavior, e.g. when we update the Python version.

Thanks for tracking all this stuff down. As is the case sometimes with the bounties, this one doesn't seem feasible to ship right now. Let's just merge the "SetOption('warn', 'all')" but commented out and a link to the issue you linked above it, then we can revisit once SCons addresses this upstream. Once that's done, consider this bounty solved!

@adeebshihadeh adeebshihadeh merged commit f820b7c into commaai:master Jan 11, 2024
2 checks passed
@adeebshihadeh
Copy link
Contributor

@bongbui321 want to try one of the bounties here? commaai/panda#1770

should be relatively straightforward and if you want, there's lots of bounties to be made from fixing these https://github.com/commaai/panda/pull/926/files#diff-999690623aab48a8f875013ed611bf2726efc87b076bbbd09ad7bb6d6dbeaec7R15

@bongbui321
Copy link
Contributor Author

bongbui321 commented Jan 11, 2024

Will have a try.

@bongbui321 bongbui321 deleted the warn_all branch January 11, 2024 21:37
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.

2 participants