Build CLI Improvements #2069
Merged
Build CLI Improvements #2069
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AmmarAbouZor
force-pushed
the
build-cli-improvements
branch
3 times, most recently
from
d2c5a19
to
8a23320
Compare
|
I think all the points in the road map has been implemented and this PR is ready for review. GitHub Workflows:
|
DmitryAstafyev
approved these changes
Aug 23, 2024
| tokio::select! { | ||
| main_res = main_process(command) => { |
There was a problem hiding this comment.
Does it make sense to use here join as soon as you are canceling both hands in any way? With select you can probably miss a fact of incorrect behavior of main_process, because main_process will be shutdown with select in any way. With join you have to wait reaction of main_process hand and it will allow notice possible issues with shutdown.
There was a problem hiding this comment.
I think using select!() for external cancelling all running jobs would make more sense for the reasons:
- All spawned processes are registered in the
task trackerand then we will be waiting for them to stop in the methodgraceful_shutdown()considering the timeout as well. - With this approach we don't need to check for cancellation in many places within the main process after the
jobs_runnerreturn like before persisting the checksums or printing the reports. Select!()isn't used withing a loop here actually, therefore we don't need to deal with the cancellation safety concerns like whenselect!()is used in the main app loop.
This solution is based actually on the Tutorials from Tokio crate
This is still missing from the build, I'll add it and test it in this PR |
|
@DmitryAstafyev Thanks for pointing out the I fixed those issues in the last commit, can you please give it a quick review too 🙏 |
AmmarAbouZor
force-pushed
the
build-cli-improvements
branch
from
b3d912d
to
a2e4d46
Compare
kruss
reviewed
Aug 29, 2024
kruss
reviewed
Aug 29, 2024
kruss
reviewed
Aug 29, 2024
kruss
reviewed
Aug 29, 2024
kruss
approved these changes
Aug 29, 2024
|
@kruss @DmitryAstafyev Could you please have a look on the last commit. I combined task singletons into one singleton only. |
DmitryAstafyev
approved these changes
Aug 29, 2024
* Add fail_fast CLI option to return early if any job fails * Fix handling if a job has failed by checking the job status besides if the job is error or not. * Change static constants to consts
* Add no-ui option to CLI arguments * Change tracker initialization logic to take no-ui argument and skip initializing and using the UI elements once it's activated * Extra logging is still missing and implementation could change one the options for build logs are Introduced
* Initialization for tracker and fail_fast must be set for Clean command as well because it uses the tracker and the jobs runner system too.
* Commands output on stderr should be sent to the tracker to make all logs visible to users while the app is running and to enable the tracker from handling the logs in one place
* All logs will be sent to the tracker to be shown on UI and saved in the logs cache at the same time. * Tracker can handle the different cases of logging at one place * Tracker spawns to main tasks: one for UI bars and the other for logs. * Remove returning results from run_ui task since it's spawn and its result can't be handled. * Remove unnecessary else clause inside tokio::select!() macro in the spawner.
* UI and logs have now the following four options:
- Show UI bars and print errors only.
- Show UI bars and print report for all jobs when all of them
have finished.
- Suppress UI and print the report of each jobs once it's finished
directly
- Suppress UI and print each log immediately once it's emitted,
printing the job names with each log.
* These options can be configured with CLI arguments replacing the
report and the `no-ui` arguments
* Export to file directly has been removed to avoid the problem with
printing the ANSI colors characters since all shells are efficient
with piping the output into a file.
* Methods for writing the reports has been moved to their own module and
has been improved.
* Make the code more simple by merging never ending async block and the ending one into one block. * With this refactoring we don't need the `future_lite` dependency anymore
* Use cancellation token and tasks tracker to control how task will shutdown and process will be killed once a job fails * Cancellation token and tasks tracker are created in the job runner for now but it may be moved in the future once graceful shutdown is implemented for Ctrl-C signal * Small refactoring for print method on the tracker. * Typos
* General cancellation components are made static across the app for easy access, since they suppose to control spawning the tasks of the main process. * This move will make it easier to implement a graceful shutdown for Signals like <Ctrl-C>
* Cache shutdown signals `<Ctrl-s>` to try to kill all the tasks before existing then shutting down the tracker ui and log channels to make sure all channels has been closed before exiting the app. * The implementation of the actual main function is moved to a separate function leaving the main function to handle caching cancel signal along side printing showing the results of the main function once it has finished. * Cancelling must be implemented manually on wrapper tests since they run synchronously in a loop that must be existed manually. * Adjustments of the jobs runner main loop to ensure no message is sent to tracker in case of manually shutting down
* Use colors in the report for the job status * Use cyan instead of blue in the UI bars for better reading * Job results lines will be printed in immediate mode only since we are printing the results in the reports for other modes. * Job results are printed with bold font.
App is an ambiguous name and it's better to have the name explicitly.
* CLI command for release is created with its arguments. * Release module with the main command and its logs are implemented. * Clean and Bundle with snapshot functions are done. * Compress function is in progress: - Version can be retrieved from electron package.json file - Build path and compression is missing. * Documentation for CLI commands are missing.
* Compression command is implemented * Add development CLI flag & CLI documentation * Small fixes & UI improvements
* Add missing setting environment variables. * Add timer for the part of building chipmunk too, since we are not showing the UI bars. * Check if release directory exist before deleting. * Persist build checksum records since we are building Chipmunk in the release. * Fix converting verbose cli argument to ui mode. * Remove unused resource path function * Remove forgotten todo!().
* Fixes for compressing:
- compressing command uses shell wildcards `*` and therefore we must
call the shell in the command and pass the command and its arguments
as one long argument for the command.
- This solution can be replaced by doing the compressing directly form
within rust.
- Fix retrieving version name.
- Remove forgotten todo
* Fixes on Window: Electron app path has a special case on windows and can't be retrieved without the shell with its shell name. * Remove todo since `tar` exists on windows by default as well and it would better to not include it in the tool itself if possible.
* Ensure compressed file has been created and print its path to console. * Small refactoring and changing method names. * Add missing logging for start building chipmunk. * Add documentation for the newly created modules. * Fixing aliases manual printing and add comment about the open issue for visible aliases on clap crate
It makes since to activate fail_fast by default on the run command since the users would activate it more likely in this scenario.
* Logs on the library can be useful when the library is used within an app that has logging functionality within. * Add logs are on the tracing level.
* Set the main documentation file to the README.md * Add documentation on module level for each module. * Provide messing documentation for public functions and types. * Remove unused Enum. * Small renames & Comments
* Integrate UI options the commands * Integrate fail fast in commands
Added small section with short description to the tool, providing a link to its own README file.
* This change lead to adding extra clause on filtering out the not included jobs, which is checking if the job is included in the original associated targets with the main job to avoid running linting or testing on Core if the user wants to run linting or testing on Bindings and Wrapper only. * Unit Tests has been adjusted accordingly.
* Use the UI option "print" to print the logs of each jobs once it's done.
* Created GitHub to create test releases using the release command with the build CLI tool. * This action can be invoked manually only and doesn't checks for refs * Release name will won't have the refs within it * Distribution part is deactivated too.
* Move Copying Client files to App to before start building App. * Add missing copying for the file 'package.json' to dest directory after App build is done. * Fix typo
Make sure to remove `client` directory from App before renaming client files to it.
`yarn install --production` is deprecated on the used version on yarn.
Manage keeping track on spawned jobs, cancellation, shutting down and fast fail in one struct that is used as a singleton.
AmmarAbouZor
force-pushed
the
build-cli-improvements
branch
from
c146f71
to
9b65024
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR closes #2019 and it still work in progress.
RoadMap:
BindingandWrappertargets.fail fastflag to the tasks to enable early return once one task has failedcargo docpackage.jsonfile todestdirectoryUpdates:
Logs and UI changes:
UI and logs have now the following four options:
These options can be configured with CLI arguments replacing the report and the
no-uiargumentsExport to file directly has been removed to avoid the problem with printing the ANSI colors characters since all shells are efficient with piping the output into a file.
Methods for writing the reports has been moved to their own module and has been improved.
Release command:
Adding Rust core to binding dependencies
CoreTarget has been added as a dependency forBindingandWrapperand changes has been made to avoid calling lint or test onCoretarget in case when the user want to run lint or test on the binding targets only.Integration with GitHub Actions