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

interpreters/python: Avoid warnings that could be treated as errors #2886

Merged

Conversation

tmedicci
Copy link
Contributor

Summary

  • interpreters/python: Avoid warnings that could be treated as errors and prevent CI from failing when EXTRAFLAGS="-Wno-cpp -Werror" is set.

Please check the discussion at apache/nuttx#15099 (comment)

Impact

Avoid CI failing when EXTRAFLAGS="-Wno-cpp -Werror" is set

Testing

Internal CI testing + test with CI's docker image according to apache/nuttx#14601 (comment):

sudo docker run -it nuttx ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest \       
  /bin/bash -c "
  cd ;
  pwd ;
  git clone https://github.com/tmedicci/incubator-nuttx --branch feature/python nuttx;
  git clone https://github.com/tmedicci/incubator-nuttx-apps --branch improvement/python_gcc_warnings apps ;
  pushd nuttx ; echo NuttX Source: https://github.com/apache/nuttx/tree/\$(git rev-parse HEAD) ; popd ;
  pushd apps  ; echo NuttX Apps: https://github.com/apache/nuttx-apps/tree/\$(git rev-parse HEAD) ; popd ;
  cd nuttx/tools/ci ;
  (./cibuild.sh -c -A -N -R testlist/risc-v-06.dat || echo '***** BUILD FAILED') ;
"

@nuttxpr
Copy link

nuttxpr commented Dec 10, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it could be improved.

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change. Referencing the related discussion is helpful.
  • Impact is Addressed: The impact section highlights the primary benefit (preventing CI failures). While it focuses on the build, it implies the broader impact of improving CI stability.
  • Testing Provided: The testing section includes the build hosts (implicitly Docker/Linux) and targets (RISC-V). The commands to reproduce the testing are a significant plus.

Areas for Improvement:

  • Impact Specificity: While the impact on the build is mentioned, explicitly stating "NO" or "YES" for each impact category (user, hardware, documentation, security, compatibility) would improve clarity and completeness. Even if the answer is "NO," stating it explicitly avoids ambiguity.
  • Testing Logs Missing: The testing section mentions "Testing logs before change" and "Testing logs after change," but the code blocks are empty. Including actual logs demonstrating the issue before the change and the successful resolution after the change is crucial.
  • Target Specificity: The target description is a bit vague ("RISC-V"). Specifying the specific RISC-V configuration (e.g., qemu-rv32ima) would be beneficial.

Recommendation:

Populate the missing log sections with actual output. Explicitly address all impact categories, even if the answer is "NO." Clarify the target architecture/board/configuration. These improvements will make the PR easier to review and increase confidence in the change.

@tmedicci tmedicci changed the title interpreters/python: Avoid warninns that could be treated as errors interpreters/python: Avoid warnings that could be treated as errors Dec 10, 2024
@tmedicci tmedicci force-pushed the improvement/python_gcc_warnings branch from 53e81f2 to 83e3aa1 Compare December 10, 2024 18:17
This commit disables some warnings when building CPython to avoid
CI failing when `EXTRAFLAGS="-Wno-cpp -Werror"` is set.
@tmedicci tmedicci force-pushed the improvement/python_gcc_warnings branch from 83e3aa1 to 8630408 Compare December 10, 2024 18:21
@xiaoxiang781216 xiaoxiang781216 merged commit d7ed692 into apache:master Dec 11, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants