CLI Tool for Build System #1985
Merged
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
- Display is more suitable when printing infos for the users
- Use rust built-in path methods - Change method signature to return Path instead of String
- Function is rewritten in a more idiomatic way without having to allocate memory and keeping track of the length manually
- Use Path built-in methods to set the path of the build command
- Module for wasm management have been created with the needed implementation to integrate in the system. - Some Todos are left to be clarified later.
- Method to run the tests has been implemented to use when running the tests is built in the cli app
- TODOs has been removed after clarification - Set Color flag to always in build command
- Test Cli Command defined and implemented - Methods for tests in Manager trait has been defined and implemented - Test commands in wasm module added
- Spawn Options has been added to provide more control over how spawn call should behave. - It's used to suppress sending messages in wasm npm test command because it caused weird behavior in the progress bars because when too many lines are sent at the same time
- Rewrite jobs alignment to keep the braces and the symbols in the same horizontal alignment among all lines - Small refactoring to get the length or max
- Change job numbers placeholder from zero to spaces in tracker
- Job duration is moved to the prefix part of the bars. - Total time is appended to the results.
The states of jobs has been growing and getting too complex for a tuple
The padding of finishing time can change on job ends which led to miss-alignment in the bars in some cases
Some build commands send big part of their infos to stderr than stdout, which gives the app a little more responsive looking if we update the bars on stderr too
Better names for the variables
- Bounded channels with 1 capacity is replaced with oneshot channels. - Unused Clone is removed from Tick enum because the oneshot channels aren't clone
- Native Async trait can't be applied in near feature
|
Originally created PR with useful discussion |
- The trait `RemoveDuplicates` is removed in favor of using the dedup method from the standard library
- Boolean is more representative of the release flag since it has two states only
|
AmmarAbouZor
force-pushed
the
build-cli-tool
branch
from
22c03f2 to
3d711e1
Compare
- Use the CLI tool with GitHub Actions for Pull Requests along side with the current ruby actions for comparison - Install latest rust manually instead of the deprecated GitHub Action - Use Cargo cache action for speed up installing the build CLI tool - Dependencies are installed manually for now but this can improved
AmmarAbouZor
force-pushed
the
build-cli-tool
branch
from
3d711e1 to
27c5707
Compare
|
@DmitryAstafyev @marcmo |
- Install command will always run in develop mode initially and then the command will rerun in production mode if it's active after the build is done. - This problem was solved by setting the production flag on false always for install job definition which lead to problem by calculating the checksums because production flag on install was always false leading to loading the checksums on development always even on production builds - The issue is solved by assigning the production flag in the internal command since we are calling it anyway with the production flag in the reinstall process
- Production and development use the same artifacts which will lead to false positives when the artifacts are modified via another build but the checksum of source files still the same. - To solve this problem we will reset the checksums of the opposite build production type when build is involved in the current process
Move the new CI checks using the CLI tool to their own GitHub action that will run manually only since those jobs are still work on progress and not needed to run along side with the current CI checks using rake commands
* Use Enum instead of boolean for the checksum comparison of the given target to improve code readability and avoid potential bugs * Fix Typo & Improve documentations
* Remove the unnecessary build steps from rust targets for running tests * Remove the build steps from TS targets that don't have tests and aren't dependencies of other TS targets that have tests. * Add assertion for that case if we add tests to those targets in the future. * Refactor the function to filter out the not included jobs by moving the additional filter to a closure that will run after the standard filter which allowed us to add the assertion mentioned above
* Replace `matches!()` macro and catch-all place holder in match statements for targets and job types with explicit enum types to enforce handling new types to targets and job enums. * The build system has grown to be complicated, therefore it's useful to get assistant from the compiler when new targets or job types are added
|
Hi @marcmo,
|
* Remove async from the declarations of functions that don't have await inside of them
* Use the value itself instead of a reference for target enums because their size is only 1 Kilobyte, which make it more efficient to copy it instead of copying a reference (usize) to it. * This applies to JobDefinition and DevTool enums as well
* Add comments in code to emphasize that explicit match arms with targets and job types should be used for keep the compiler assistance when adding new targets or job types. * Use reference for a string instead of moving the whole string since it's not consumed inside the function.
marcmo
reviewed
Aug 7, 2024
application/apps/rustcore/ts-bindings/spec/session.jobs.spec.ts
Outdated
Show resolved
Hide resolved
* Remove redundant test * Use assigning to _ to directly drop file after creating it
* Use one function to create process commands with yarn as cmd and the given arguments and use it in code when needed * Remove TODO comment from ts testing file and improve the relating comment for it in `wrapper.rs file
|
@marcmo Requested changes are implemented/answered. The PR can be reviewed again 👍 |
Tokio Commands has the option to kill the underline process when the command is dropped. This will provide more grateful cancellation and shut down functionality
Use { .. } for wild card matching for JobType where production value
should be ignored.
marcmo
approved these changes
Aug 9, 2024
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 the issue #1966.
This PR continue the great work in the PR #1967.
At the end of this drafts we should have a working CLI tool to build, lint, clean and test the project to replace the current solution using rake
Here is the road map for this PR:
Managerwith the enumTargetprint_dotsto visualize the dependencies graphUpdatetarget (Build, Clean...)