Team-4-submission #8

waynewangyuxuan · 2024-10-08T01:34:05Z

Merged Commits into 2

kyotov · 2024-10-09T01:42:21Z

do you have a CI link i can look at?

waynewangyuxuan · 2024-10-09T01:46:32Z

https://app.circleci.com/pipelines/circleci/XuhSKKLtTmft9W3VV6Pa6t/Pht6yzSCo6GJvUbVC1gNV8 Hi professor, this is the link.

…

On Tue, Oct 8, 2024 at 9:42 PM Kamen Yotov ***@***.***> wrote: do you have a CI link i can look at? — Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYEIBD257F3ZB5GC66ZO46TZ2SCZFAVCNFSM6AAAAABPRDIMGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBRGEYDKOJQHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

kyotov · 2024-10-09T01:50:38Z

seems like you can do a bit more to integrate this part?

waynewangyuxuan · 2024-10-09T02:08:14Z

We overlooked this part before… I think we can. Btw, the ci/cd on master, the (most recent ) failed one, is what we intend to show you. The ci/cd did not past because we did not pass the clang-tidy check. We put the clang-tidy into crest, wondering whether we should do it at build or do it with ctest like it is?

…

On Tue, Oct 8, 2024 at 9:51 PM Kamen Yotov ***@***.***> wrote: seems like you can do a bit more to integrate this part? image.png (view on web) <https://github.com/user-attachments/assets/e4960487-f149-484f-9a27-b447d4c4714c> — Reply to this email directly, view it on GitHub <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AYEIBD3FO4A3QEF2PGIDTZ3Z2SDYJAVCNFSM6AAAAABPRDIMGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBRGEYTEOBYGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

kyotov

please address the comments and let me know when it is ready for another look

kyotov · 2024-10-30T01:25:17Z

.circleci/config.yml

+jobs:
+  build-test:
+    docker:
+      - image: cimg/base:current


for extra credit you can build your own image, but for this hw, this is ok

kyotov · 2024-10-30T01:25:41Z

.circleci/config.yml

+      - run:
+          name: Build
+          command: | 
+            echo "Building"
+            mkdir build && cd build
+            cmake -S ..
+            make -j $(nproc)
+            cd ../
+            echo "Built"
+      - run:
+          name: Test
+          command: | 
+            echo "Start Testing"
+            cd build
+            ctest
+            echo "Testing Done"


where is the clang-tidy running?

kyotov · 2024-10-30T01:26:18Z

CMakeLists.txt

+  set(NINJA_TEST_SOURCES
+    src/build_log_test.cc
+    src/build_test.cc
+    src/clean_test.cc
+    src/clparser_test.cc
+    src/depfile_parser_test.cc
+    src/deps_log_test.cc
+    src/disk_interface_test.cc
+    src/dyndep_parser_test.cc
+    src/edit_distance_test.cc
+    src/elide_middle_test.cc
+    src/explanations_test.cc
+    src/graph_test.cc
+    src/json_test.cc
+    src/lexer_test.cc
+    src/manifest_parser_test.cc
+    src/missing_deps_test.cc
+    src/ninja_test.cc
+    src/state_test.cc
+    src/string_piece_util_test.cc
+    src/subprocess_test.cc
+    src/test.cc
+    src/util_test.cc
+	)


this looks like something that should be in the original ninja?
what is the purpose?

Oh, we will delete this in here, we did not write any code at the time, so we were checking the src code see whether clang-tidy was working

kyotov · 2024-10-30T01:26:55Z

CMakeLists.txt

+   if(CLANG_TIDY_EXE)
+    add_test(
+        NAME clang-tidy-check
+        COMMAND ${CLANG_TIDY_EXE} ${NINJA_TEST_SOURCES} --checks=-*,modernize-* --warnings-as-errors=*
+    )
+    else()
+    	message(WARNING "clang-tidy not found, skipping clang-tidy test")
+    endif()


i see that you run it now, but this is not the normal way to run clang-tidy in cmake (i think)...
can you check?

I think you are right about this, we will update this

waynewangyuxuan · 2024-11-07T00:14:40Z

please address the comments and let me know when it is ready for another look

Hi, professor, the use of clang-tidy is fixed

kyotov · 2024-11-07T01:20:54Z

.circleci/config.yml

-
+          name: Run Tests
+          command: |
+            ./cicd-build/ninja_test


use ctest
add perf tests

kyotov · 2024-11-07T01:21:33Z

steps/hello-world-graph/libninja.dylib

this should be in gitignore

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.

CircleCI Commit Update CI update README.md add sudo Updated CMakeLists.txt to include clang-tidy configuration change clang test name update clangtest file update clang-tidy/circleCI use. Learned from team5's implementation update yml file typos init CircleCI Commit Update CI update README.md add sudo test folder wrong

Added ctest and performance test. Try to show test on CI/CD dashboard, followed the instruction, however we could not get it working somehow.

kyotov requested changes Oct 30, 2024

View reviewed changes

kyotov reviewed Nov 7, 2024

View reviewed changes

waynewangyuxuan force-pushed the master branch from 1793eeb to 84f27af Compare November 10, 2024 01:40

digit-google and others added 4 commits November 9, 2024 20:44

Add InputsCollector class.

5f5474e

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.

04a73ad

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

waynewangyuxuan force-pushed the master branch 2 times, most recently from 516d7b4 to eaa07e0 Compare November 10, 2024 20:22

add test result display for circleCI dashboard

75a9412

Added ctest and performance test. Try to show test on CI/CD dashboard, followed the instruction, however we could not get it working somehow.

waynewangyuxuan force-pushed the master branch from eaa07e0 to 75a9412 Compare November 10, 2024 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Team-4-submission #8

Team-4-submission #8

waynewangyuxuan commented Oct 8, 2024

kyotov commented Oct 9, 2024

waynewangyuxuan commented Oct 9, 2024 via email

kyotov commented Oct 9, 2024

waynewangyuxuan commented Oct 9, 2024 via email

kyotov left a comment

kyotov Oct 30, 2024

kyotov Oct 30, 2024

kyotov Oct 30, 2024

waynewangyuxuan Nov 3, 2024

kyotov Oct 30, 2024

waynewangyuxuan Nov 3, 2024

waynewangyuxuan commented Nov 7, 2024

kyotov Nov 7, 2024

kyotov Nov 7, 2024

Team-4-submission #8

Are you sure you want to change the base?

Team-4-submission #8

Conversation

waynewangyuxuan commented Oct 8, 2024

kyotov commented Oct 9, 2024

waynewangyuxuan commented Oct 9, 2024 via email

kyotov commented Oct 9, 2024

waynewangyuxuan commented Oct 9, 2024 via email

kyotov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waynewangyuxuan commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment