Team 2 - Converter; Able to convert hello world build.ninja into build.ninja.cc - #18
Team 2 - Converter; Able to convert hello world build.ninja into build.ninja.cc#18mayank-ramnani wants to merge 33 commits into
Conversation
GitHub Actions: Prevent ctest invocation error on macOS
This adds `override` statements, and removes an un-necessary
destructor declaration from status_printer.h in order to remove
annoying compiler warnings when building Ninja tests with CMake
with recent compilers. For example, clang 16.0 complains with:
```
/path/to/ninja/src/status_printer.h:29:16: warning: 'EdgeAddedToPlan' overrides a member function but is not marked 'override' [
-Winconsistent-missing-override]
virtual void EdgeAddedToPlan(const Edge* edge);
^
```
The root issue is that while libninja is compiled explicitly for C++11,
which disable the warning, the tests themselves are not, since they rely
on GTest which now requires C++14, and thus the standard being used is
determined by the compiler's default configuration.
This field is unused. This patch removes compiler warnings when building with recent compilers (e.g. Clang 16.0)
It was declared in an anonymous namespace, and nothing uses it anymore, removes a compiler warning.
Technically, these are not required since Ninja is built with -std=c++11, but using these directives is good practice to clarify the code and avoid simple human errors.
Remove warnings
Add a new class to wrap the logic needed to implement the `inputs` tool correctly (see Issue ninja-build#2482 for details), and provide a unit-test for it.
This uses the InputsCollector class introduced in the previous patch to implement the tool properly. Results are still shell-escaped and sorted alphabetically. Fixed ninja-build#2482
Add new options to the `inputs` tool in order to change the format of its output: - `--no-shell-escape` to avoid shell-escaping the results. - `--dependency-order` to return results in dependency order, instead of sorting them alphabetically. - `--print0` to use \0 as the list separator in the list, useful to process the target paths with `xargs -0` and similar tools.
Use C string macros to hold ANSI escape sequences in order to make the test much easier to understand and follow. NOTE: This does not change the test in any way!
Move the function to its own source file since it is no longer trivial (and will get optimized in future patches). + Add a new ElideMiddleInPlace() function that doesn't do anything if no elision is required + use it in status.cc. + Add a new elide_middle_perftest program to benchmark the performance of the function. On my machine, the 'avg' result is 159.9ms.
Add a fast-path for the case where there is no escape sequence
in the input, that avoids un-necessary string allocations.
Properly handle ANSI color sequences that appear in the elided
part of the input string. They *must* be preserved to ensure
that the right span is printed with the right color.
For example, consider the following input:
|GREEN|aaaaaaaaaaaaa|RED|bbbbbbbbbbb|BLACK|
With different levels of elision:
max_width=0 -> |GREEN|RED|BLACK| instead of ""
max_width=5 -> |GREEN|a..|RED|b|BLACK|
(this was |GREEN|a..b|BLACK| before this fix)
Moreoever, do not call std::regex_iterator::str() to avoid
un-necessary string allocations and concatenations.
With this patch, 'elide_middle_perftest' goes from 159.9ms
to 87.1 ms on my machine (x1.8 faster)
Get rid of std::regex and std::vector usage in ElideMiddleInPlace() function. These are replaced with custom iterator classes that do not perform any memory allocation at runtime. Instead the input string will be parsed twice (once to determine the visible width, and another time to create the elided result string). With this patch, elide_middle_perftest goes from 87.1ms to 15.0ms on my machine (x5.8 faster!). This also removes 65 kiB from the stripped Linux binary for Ninja (!)
Just check for the ESC character in the `output` string, if none is present, or if the terminal supports color, avoid unnecessary busy work (string allocations and parsing).
…ddle Faster elide middle implementation
For most actions, the depfile will be in the same directory as one of its outputs, and their parent directory will be created by Ninja before running the command. However, this is not always the case. In particular, the GN build tool is changing its Ninja build plan generation logic, switching from using stamp files to phony targets, after issue ninja-build#478 was fixed in Ninja. For additionnal context see https://gn-review.googlesource.com/c/gn/+/11380. The newly generated build plans trigger this condition more frequently, which results in flaky build failures for large GN-based projects such as Fuchsia. This patch ensures the depfile's parent directory is always created before the command is launched to get rid of the issue entirely.
…directory Ensure depfile's parent directory is created before running an action.
Fix `inputs` tool logic and add new formatting options.
…while using ninja parser
…a separate binary; Add debug build script for converter based on ninja
|
Review from Daniel Torres, Lucas Eng, Maksym Yemelianenko Change Granularity: Code Functionality: Readability & Structure: Testing: Overall the code is well-executed, meeting the specified requirements, adhering to standards, and showcasing clear organization. With minor adjustments in modularity and documentation, this PR would be even stronger. Great job! |
There was a problem hiding this comment.
Additional Team 4 review:
Initially we started by ignoring archive/convert.py since it seemed that It is outdated (and it was mentioned in PR as well, so no questions regarding that). then we checked converter/src/ files and saw an implementation that was after the python version and we left several comments for enhancing the idea if you would like to follow it through. after that we realized that the actual work was done in just /src directory. therefore, these are our recommendations:
Change Granularity
- while all commits are clear and very granular, having the approximately same size and serving the clear purpose, we believe it also creates some confusion. we can see that the team tried different approaches and developed their way to a better approach, but in our perspective, the process of finding the approach could be hidden (at least squashed in a singular commit in history and deleted from the branch so it is visible what is up to date and what is outdated)
Code Functionality
- code is functional and the flow is concise, it runs for all the versions (with minor tweaks) and
Readability & Structure
- lots of explanation comments, general code is nice and clean, seems to have no obvious bugs
- has some magic constants here and there, but generally does not seem to be of a bad quality. the opposite, the code is easy to read and understand and has a high quality for fellow programmers to comprehend in our opinion.
- modularity is good for all the versions
Standards Compatibility
- the code has consistent linting standard
- API is clear for the CPP implementation, for manifest, it is irrelevant
Testing
- testing is the weakest part of the PR, since they do not exist. the only test is an actual run on a ninja file with some flags and some ninja usage, but it is not comprehensive enough to test the functionality in our opinion.
- unit tests do not exist and therefore we strongly encourage to have at least some basic parts covered in tests to ensure consistency and unbreakability during scaling.
this was the general overview based on guidelines. in the end we think that while it has some flaws (as mentioned), the code satisfies a good quality checkmark on this stage of development. our recommendations are purely based on our experience as developers and should be taken as a suggestions rather than judging and critique.
Co-authored-by: deflucaseng lucaseng19@gmail.com
ec109b0 to
d497a69
Compare
|
This PR can be closed. |
7 participants
Initially, implementation of converter was done in Python. That is also included in this PR for archival purposes.
The current, working implementation of the converter is done in C++, by doing changes to the Ninja source code itself.
Below are the instructions for how to use it.
Build
This will create the converter in the
debug-builddirectory with debug symbols.Run
Run the above in a directory where a
build.ninjafile is presentTesting
This will run the converter on the hello world example
build.ninjafile and produceoutput.ccas a result.