Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Team4-Implementation-Final #24

Open
wants to merge 4 commits into
base: team-4-graph
Choose a base branch
from

Conversation

waynewangyuxuan
Copy link

@waynewangyuxuan waynewangyuxuan commented Dec 6, 2024

Shadowdash

  1. Graph Building
    This is branch is a experimental feature that deconstrucsts and demonstrates how Ninja create it graphs from buiding a simple new-graph/hello_world.cc from scratch.
    Do the following:
cd new-graph
chmod -x easybuild.sh
./easybuild.sh

Or your can do it manually:
a. Before building simulate-ninja-graph, you need to compile and generate a shared object that includes partial functionality (functions) of Ninja. You can refer to lines 41 to 67 in new-graph/easybuild.sh to find the .cc files that need to be included.
b. After generating the shared object, compile the new-graph/simulate-ninja-graph.cc file and link it with the previously built shared object.
c. Once completed, run ./simulate-ninja-graph and pass hello_world as an argument (./simulate-ninja-graph hello_world). This program will look for a new-graph/hello_world.cc file in the current folder and compile it.
d. After the compilation, an executable file named new-graph/hello_world will be created, which you can run.
(You can modify easybuild.sh based on your OS and compiler to simplify this process.)

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.
change getopt.h so that it looks for local opt if the system is linux or
macOS

fixed readme
@waynewangyuxuan waynewangyuxuan changed the title Team4-MiniTask-Submission-Final Team4-Implementation-Final Dec 10, 2024
@kyotov
Copy link

kyotov commented Dec 12, 2024

chmod -x easybuild.sh
You can do this in the repo and commit it so you don't have to ask users to do it.

@kyotov
Copy link

kyotov commented Dec 12, 2024

still feels like there are a lot of parts of this diff that are not part of the task at hand!

@@ -0,0 +1,68 @@

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of this file is not specific to the task at hand.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusted at #28

@@ -10,6 +10,7 @@
/build_log_perftest
/canon_perftest
/clparser_perftest
/elide_middle_perftest
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this part of this task?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjusted at #28

@@ -3,6 +3,13 @@ cmake_minimum_required(VERSION 3.15)
include(CheckSymbolExists)
include(CheckIPOSupported)

find_program(CLANG_TIDY_EXE NAMES clang-tidy)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not look like part of this task?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, we have it updated at #28

@@ -228,6 +228,71 @@ def test_tool_inputs(self) -> None:
out2
''')

self.assertEqual(run(plan, flags='-t inputs --dependency-order out3'),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this your code?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is the ninja code

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, we can get it removed

Comment on lines +16 to +19
command.AddText("g++");
command.AddText(" ");
command.AddText("-c");
command.AddText(" ");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be a single AddText, right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, updated at #28

@@ -526,7 +526,7 @@ void Plan::ComputeCriticalPath() {
for (Edge* edge : sorted_edges)
edge->set_critical_path_weight(EdgeWeightHeuristic(edge));

// Second propagate / increment weidghts from
// Second propagate / increment weights from
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this your code or ninja had typos?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not sure, but we have it updated at #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants