Team 2 New Language Test Files #12

mayank-ramnani · 2024-10-16T20:19:08Z

@ Marwan AbdElhameed
@ Ricky Zhang

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.

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.

fix typos

- build dependencies if they dont exist

jazwu · 2024-10-31T00:40:13Z

For future commits, consider adopting the <type>: <summary> format (e.g., feat: add new language or fix: substitute only first input file). This structure improves readability. Also, aim to make commit messages clearer in describing what changes were made and why, which helps the team track updates more effectively

Shihuihuang1103 · 2024-11-03T05:22:09Z

new-language/main.cpp

+
+    PHONY(clean, "clean_rule");
+    RULE(clean_rule,
+         shadowdash::constant("rm", 2),


It might be helpful to have a method that calculates the length of each string instead of manually counting and passing the length.

Shihuihuang1103 · 2024-11-03T05:45:50Z

Correctness:

Confirming that bar.o, foo.o and baz.o can be compiled as expected.
Verify that phony "clean" functions as intended, effectively removing the generated .o files.

Readability:

The code is well-organized and naming conventions are consistently followed.
Clear messages are logged to the console to indicate building targets and stages.

Testing:

Unit testing could be beneficial for core methods like resolveVariable, defineRule, etc.

dl5214 · 2024-11-04T00:05:45Z

new-language/manifest.h

+        std::optional<bool> restat_{false};
+        std::optional<std::string_view> rspfile_;
+        std::optional<std::string_view> rspfile_content_;
+        std::optional<std::string_view> pool_;


Will you further consider make 'pool' a separate rule, in parallel with RULE, BUILD, VAR, DEFAULT, etc.?

dl5214 · 2024-11-04T00:17:46Z

The overall functionality looks good, and thank you for the substantial work. I would like to make some additional comments based on Jasmine and Shihui's thorough review.

Is it really necessary to add foo, bar, and baz to this project? Are they for testing purpose? If they are not related to the whole project, maybe it would be better to either remove them or put them into an archive/legacy folder. Otherwise if they are indeed necessary, please consider renaming them.
Maybe there would be benefits separating 'pool' from other rules, or you can leave it as is.
It might be helpful if you would consider providing some very simple example usage of your newly defined language in docString or through comments, like what you have done in the main function.
As Professor suggested further, please submit only one commit in this PR and remove any inrelevant changes.

…ild issues and handles creating driectories

kyotov

please checkout https://docs.google.com/document/d/11e8zR3kcm7B-D6brAH_D4OgtYBEf94GZwQIXrnti5hM/edit?tab=t.0 and address the items that apply to you. not all of them necessarily do, but some do... the i will look again

MarwanWalid2 · 2024-11-27T04:04:38Z

Newest PR: PR23

jhasse and others added 26 commits August 28, 2024 13:47

Merge pull request ninja-build#2479 from mcprat/workflow-ctest-verbose

f220dc5

GitHub Actions: Prevent ctest invocation error on macOS

explanations.h: Remove Explanations:enabled_ field.

77d979b

This field is unused. This patch removes compiler warnings when building with recent compilers (e.g. Clang 16.0)

metrics.cc: Remove unused GetFrequency() function.

0f05540

It was declared in an anonymous namespace, and nothing uses it anymore, removes a compiler warning.

build.cc: Add 'override' directives to virtual method overrides.

50ae55a

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.

Merge pull request ninja-build#2486 from digit-google/remove-warnings

4b7d399

Remove warnings

Add InputsCollector class.

7e03348

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.

Fix inputs tool implementation.

5b94d34

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

Rewrite ElideMiddle.ElideAnsiEscapeCodes test.

60f901e

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!

StatusPrinter: only strip ANSI sequences when needed.

1ba5570

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).

Merge pull request ninja-build#2487 from digit-google/faster-elide-mi…

b2ae865

…ddle Faster elide middle implementation

typos

60838c1

Merge pull request ninja-build#2492 from digit-google/create-depfile-…

fe83433

…directory Ensure depfile's parent directory is created before running an action.

Merge pull request ninja-build#2485 from digit-google/fix-inputs-tool

41ecb09

Fix `inputs` tool logic and add new formatting options.

Merge pull request ninja-build#2494 from vaerksted/master

6a31acc

fix typos

Add CircleCI integration

0e28864

Team2: Add new langauge

24168fb

fixed manifest

34f3bae

fixed "in" variable substituting only first input file

ce9d66d

- default builds are built first

dddc01b

- build dependencies if they dont exist

can execute phony commands

22d26b1

Shihuihuang1103 reviewed Nov 3, 2024

View reviewed changes

dl5214 reviewed Nov 4, 2024

View reviewed changes

mayank-ramnani force-pushed the team2-homework1 branch 2 times, most recently from ec109b0 to d497a69 Compare November 7, 2024 02:31

rzh4321 and others added 2 commits November 6, 2024 21:44

feat: added pool implementation

a2208c0

Add: zlib-test files, this contains a new manifest that fixes some bu…

41a908d

…ild issues and handles creating driectories

kyotov requested changes Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Team 2 New Language Test Files #12

Team 2 New Language Test Files #12

mayank-ramnani commented Oct 16, 2024 •

edited

Loading

jazwu commented Oct 31, 2024 •

edited

Loading

Shihuihuang1103 Nov 3, 2024 •

edited

Loading

Shihuihuang1103 commented Nov 3, 2024

dl5214 Nov 4, 2024

dl5214 commented Nov 4, 2024 •

edited

Loading

kyotov left a comment

MarwanWalid2 commented Nov 27, 2024 •

edited

Loading

Team 2 New Language Test Files #12

Are you sure you want to change the base?

Team 2 New Language Test Files #12

Conversation

mayank-ramnani commented Oct 16, 2024 • edited Loading

jazwu commented Oct 31, 2024 • edited Loading

Shihuihuang1103 Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Shihuihuang1103 commented Nov 3, 2024

dl5214 Nov 4, 2024

Choose a reason for hiding this comment

dl5214 commented Nov 4, 2024 • edited Loading

kyotov left a comment

Choose a reason for hiding this comment

MarwanWalid2 commented Nov 27, 2024 • edited Loading

mayank-ramnani commented Oct 16, 2024 •

edited

Loading

jazwu commented Oct 31, 2024 •

edited

Loading

Shihuihuang1103 Nov 3, 2024 •

edited

Loading

dl5214 commented Nov 4, 2024 •

edited

Loading

MarwanWalid2 commented Nov 27, 2024 •

edited

Loading