-
Notifications
You must be signed in to change notification settings - Fork 81
ci: 👷 check that examples in header files compile #774
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
base: develop-pros-4
Are you sure you want to change the base?
ci: 👷 check that examples in header files compile #774
Conversation
|
@Rocky14683 Can you please run the pipeline? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Updated logging code and regex pattern for code blocks.
|
@Rocky14683 can you please rerun pipeline? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@BennyBot can you please rerun the pipeline? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@BennyBot can you please rerun the pipeline? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
djava
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this asyncio approach is much cleaner, nice job.
Last request besides what comments i left was just - type hints?
| "include/main.h", | ||
| "-std=c++23", | ||
| "-I", "include", | ||
| "-D", "_PROS_KERNEL_SUPPRESS_LLEMU_WARNING", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huge nitpick (like, dont bother changing this) but for the record its UB to have any identifier (including macros) that start with underscore then a capital letter lol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually idek if you wrote that macro or not but 🤷 just having fun being an asshole by mentioning it
check_example_code.py
Outdated
|
|
||
| Args: | ||
| code_text (str): The C/C++ source code to compile. | ||
| filename (str): The name of the file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this remove a mistake?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that shouldnt have been removed
| # Generate assembly (avoids linker errors) | ||
| "-S", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the more standard -c not work? if not then probably give a longer comment to explain why youre doing -S because it took me a second for that to click
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-c would probably work, but assembling the files seems like an unnecessary step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, maybe just a better comment - something like Just generate assembly, the further compilation steps are not necessary for this CI check maybe
| stdout, stderr = await process.communicate(input=code_text.encode("utf-8")) | ||
| success = (process.returncode == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting - where do you await the compilation being done? im not super familiar with this but im confused. does process.communicate() imply blocking to wait for a response? is there a timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, communicate sends input via stdin and then waits for the process to finish running (no timeout though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok maybe a comment that says that because its not obvious imo
| "-x", "c++" if is_cpp else "c", | ||
| "-std=c++23" if is_cpp else "-std=c2x", | ||
| # Only need precompiled headers for C++ | ||
| *(("-I", precompiled_include_directory) if is_cpp else ()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird syntax trick lol maybe just do it after the list initialization
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Some silly notes I had while skimming the code:
1 seems like it would be a lot of work, but everything else seems reasonable and justifiable |
|
re @meisZWFLZ
That's probably a good idea in the long run, I didn't implement it because Makefile scares me a little.
The code compiles everything in parallel using as many cores as possible with asyncio, so I don't think doing both checks in parallel would offer much speedup.
That seems reasonable, but the script needs a few changes for development use---caching the example code for example, since the script always recompiles every example code snippet right now, which works for ci but makes it painful to repeatedly run during development. |
|
Do the other checks still run if the example check fails? I would think that would be ideal. I kinda wonder whether this should be written in the makefile rather than a python script. Might make it a bit easier to implement caching with make rules. |
Summary:
This PR adds a Python script to be ran during CI that isolates all the example code snippets in the
include/prosdirectory and checks that they compile.(Note: this script was partially written by Copilot)The current code doesn't contain any part of the original code which contained code from CopilotMotivation:
Prevents issues like #760.
Test Plan: