-
Notifications
You must be signed in to change notification settings - Fork 257
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
Updated BuildCompat.py #3199
base: Development
Are you sure you want to change the base?
Updated BuildCompat.py #3199
Conversation
added: 1. Asynchronous execution: Use asyncio.create_subprocess_exec to start processes and wait for them to complete asynchronously. 2. File reading optimization: Reading a file line by line inside an asynchronous function. 3. Asynchronous task management: Use asyncio.gather to execute commands in parallel if the -j flag is enabled. This approach allows multiple processes to run in parallel, effectively managing their termination, which should improve script performance.
You can download the rebuilt assembly for this PR here: https://combatextended.lp-programming.com/CombatExtended-9616442798.zip |
csproj = FilePath("Source").descendant(csproj) | ||
if tm and name not in sys.argv: | ||
return | ||
csproj_path = Path("Source").joinpath(*csproj.split('/')).as_posix() |
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.
It is worth noting that joinpath
does not sanitize input, so this does not enforce that csproj_path
is a child of "Source"
. It does not appear that pathlib
has a way to enforce that constraint. In this case, I don't see an obvious attack vector exposed by relaxing this constraint, and FilePath
is the only thing we're using out of tiwsted
, so removing it makes the code more portable (as pathlib
is part of the standard library).
proc = await asyncio.create_subprocess_exec(*cmd) | ||
await proc.wait() | ||
if proc.returncode != 0: | ||
raise SystemExit(proc.returncode) |
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 exits non-zero even in parallel build mode. That is probably a good thing as it would let us use parallel mode in the github action and not incorrectly ship a broken build. However it leaves the other tasks hanging. They then spew their standard out and standard error messages after the main script terminates.
The return values from run_command
are accumulated by asyncio.gather
, so run_command
should simply return the failure code, and its caller should check that error code and handle it. This could be done via try/except, assuming asyncio
can be told to wait on all even after the first exception is thrown, but that does not appear to be the default from asyncio.gather
. Printing a summary (passed / total) at the end is probably the correct solution.
added:
Additions
Describe new functionality added by your code, e.g.
Changes
Describe adjustments to existing features made in this merge, e.g.
References
Links to the associated issues or other related pull requests, e.g.
Reasoning
Why did you choose to implement things this way, e.g.
Alternatives
Describe alternative implementations you have considered, e.g.
Testing
Check tests you have performed: