Draft of cli based build system #1967
Conversation
cli/src/main.rs
Outdated
| lazy_static! { | ||
| static ref LOCATION: Location = Location::new().expect("Fail to setup location of ICSMW"); | ||
| static ref TRACKER: Tracker = Tracker::new(); | ||
| } |
There was a problem hiding this comment.
You can have a look at once_cell which advertised as the new replacement for lazy_static where the standard library is supported.
cli/src/fstools.rs
Outdated
| src.to_string_lossy(), | ||
| dest.to_string_lossy() |
There was a problem hiding this comment.
PathBuf implements Display trait, so this could be written as
format!("copied: {} to {}", src.display(), dest.display()),I think though, that display calls to_string_lossy() under the hood on most operation systems
cli/src/location.rs
Outdated
| let mut root = current_dir()?; | ||
| let mut len = root.iter().collect::<Vec<&OsStr>>().len(); | ||
| loop { | ||
| if len == 0 { | ||
| return Err(Error::new( | ||
| ErrorKind::NotFound, | ||
| "Fail to find ICSMW location", | ||
| )); | ||
| } | ||
| // TODO: better compare folders stucts or some file, like some git config file | ||
| if root.ends_with("chipmunk") || root.ends_with("logviewer") { | ||
| break; | ||
| } | ||
| if len > 0 { | ||
| len = len.saturating_sub(1); | ||
| } | ||
| root.pop(); | ||
| } | ||
| Ok(Self { root }) | ||
| } |
There was a problem hiding this comment.
Here is rewrite to the same function if you want to drop the extra allocation and keeping track of the len variable
let mut root = current_dir()?;
// TODO: better compare folders stucts or some file, like some git config file
while !root.ends_with("chipmunk") && !root.ends_with("logviewer") {
if !root.pop() {
return Err(Error::new(
ErrorKind::NotFound,
"Fail to find ICSMW location",
));
}
}
Ok(Self { root })
cli/src/location.rs
Outdated
| let path_str = path.to_string_lossy().to_string(); | ||
| path_str.replace(&LOCATION.root.to_string_lossy().to_string(), "") |
There was a problem hiding this comment.
We can use the built in methods from PathBuf
if let Ok(relative_path) = path.strip_prefix(&LOCATION.root) {
relative_path.to_string_lossy().to_string()
} else {
// TODO: Should we panic here or return an error?
path.to_string_lossy().to_string()
}
|
@AmmarAbouZor you are welcome to create a PR to my fork with your suggestions. Please, let me know if there are some kind of permissions issues. |
|
@DmitryAstafyev I created a PR to your fork with the small refactoring mentioned here. I'll have a deeper look on maybe create then other PRs with farther optimizations |
- 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
DmitryAstafyev
force-pushed
the
build-cli
branch
from
bd5a4c2 to
1963678
Compare
|
@AmmarAbouZor thanks for your changes, all were merged. I've also added a couple of small changes to make possible to build a solution. and Commands and features:
Logs and output:
Bugs (probably) / features:
change to
|
Currently the progress bar will be update on each tasks each time the running command outputs a line to stdout, which is inconsistent and could lead to the feeling that something is forzen when a command doesn't output anything to stdout for a while. We can change the behavior entirely be binding the progress bar to a timer which emits events with fixed interval. However, this could lead to false positive when a command is really frozen but the progress bar is keep moving. I think it's a trade-off here but I lean to the timers since the command doesn't hang that often. I'll start with the native async-trait and the other UI stuff, then I can build the timer if the final decision is in this direction or then we can see how to implement the logs in the best way possible. |
|
@DmitryAstafyev I've provided the first PR for the UI improvements. When it's merged I can implement making the progress bar to the left moving always using a timer as mentioned above if we decide to go with that suggestion |
- 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
|
@AmmarAbouZor looks really good. Take a look at this piece of code. We are "catching" The problem is - that we actually don't get data from Well as soon as this issue is resolved, we will not have an issue with updating of progress bar anymore. Could you try to resolve it? |
|
@DmitryAstafyev I've found the problem. It's actually in the command # This will output about 5 lines
yarn run build > output.txt
# This will output all the lines because we are piping both stdout and stderr
yarn run build &> output_and_err.txtWe can also add the flag --verbose to the build command like To solve this problem we can listen to both stdout and stderr inside a select macro. Should I implement this solution in a new PR or do we need more organized way to save the data (like saving stdout and stderr separately) ? |
|
@DmitryAstafyev I was playing to test the possibilities for that and I made two prototypes one updates the bars on both stdout and stderrand the other uses a timer to update the bars regularly with a constant interval. I've created two branches on my fork for each prototype and here are links so you can get a feeling on each solution: I don't think updating on both stdout and stderr made a big difference because the commands seem to send their updates on big chunks which didn't gave us big impact on the feeling for the progress bar. You can check them and pick one then I refine it and create a PR on the wanted branch |
|
Hello @AmmarAbouZor Indeed you are welcome to make some refactoring handling As for timers/intervals - no need to add it. One more thing, which would be nice to change/add - migrate from |
|
@AmmarAbouZor okay let's make a plan of an integration: Stage 1
Stage 2
|
|
@DmitryAstafyev I've provided a PR for updating the progress bars on both stdout and stderr, and will take care to migrating form async-std to Tokio before we start with the next steps. We still have the following steps from the previous comment:
Should we add/merge them to the current plan, or they aren't relevant anymore? |
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
No description provided.