Skip to content

issue-130 copy contents related x.py from rust-forge to rustc-guide #195

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

Merged
merged 4 commits into from
Sep 29, 2018

Conversation

rajcspsg
Copy link
Contributor

@rajcspsg rajcspsg commented Sep 7, 2018

No description provided.

Copy link
Member

@mark-i-m mark-i-m left a comment

Choose a reason for hiding this comment

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

Thanks @rajcspsg! Nice work :)

I left some comments. I think this is a great starting point, and just needs a bit of adjusting.

@@ -110,6 +110,44 @@ This is just a subset of the full rustc build. The **full** rustc build
- Build libstd with stage2 compiler.
- Build librustdoc and a bunch of other things.

### Build different stages
Copy link
Member

Choose a reason for hiding this comment

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

#191 Is documenting this extensively, so I think we don't need this section.

@@ -0,0 +1,42 @@
# what is x.py?
Copy link
Member

Choose a reason for hiding this comment

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

I think the information in this file can be merged into how-to-build-and-run.md.

This first paragraph (under what is x.py) could become the first paragraph of the "Running x.py and building a stage1 compiler" section.

The information on extra build flags could be merged into "Other x.py commands".

The information after line 31 is redundant with #191.

@@ -70,3 +70,26 @@ This is much faster, but doesn't always work. For example, some tests
include directives that specify specific compiler flags, or which rely
on other crates, and they may not run the same without those options.

### Run specific tests
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps remove this section title. I think these subsections fit reasonably well under Running an individual test.

@@ -70,3 +70,26 @@ This is much faster, but doesn't always work. For example, some tests
include directives that specify specific compiler flags, or which rely
on other crates, and they may not run the same without those options.

### Run specific tests

# Run only the tidy script
Copy link
Member

Choose a reason for hiding this comment

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

This should be ###... same for the others

> rustup toolchain link stage1 build/<host-triple>/stage1
> rustup toolchain link stage2 build/<host-triple>/stage2
rustup toolchain link stage1 build/<host-triple>/stage1
rustup toolchain link stage2 build/<host-triple>/stage2
Copy link
Member

Choose a reason for hiding this comment

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

We actually want the > prompts for consistency with other chapters.

@@ -0,0 +1,11 @@
# Benchmarking rustc
Copy link
Member

Choose a reason for hiding this comment

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

Hmm... @nnethercote are these benchmarks maintained and still useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't heard of these. I just tried ./x.py bench, it looks like it's basically the same as cargo bench, i.e. it runs #[bench] unit benchmarks. That might be useful for benchmarking certain components within the compiler (mostly libraries in std) but it doesn't give any info on the overall speed of the compiler. Also, I got compile errors for a bunch of these, so it looks like they aren't being run regularly.

Copy link
Member

Choose a reason for hiding this comment

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

@rajcspsg Perhaps we should remove this section/file altogether then? It seems to be outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file is removed.

You might want to build documentation of the various components
available like the standard library. There’s two ways to go about this.
You can run rustdoc directly on the file to make sure the HTML is
correct which is fast or you can build the documentation as part of the
Copy link
Member

Choose a reason for hiding this comment

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

"correct which is fast or" -> "correct, which is fast. Alternately, "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is done.

the documentation you want.

## Document internal rustc items
By default rustc does not build the compiler docs for its internal items.
Copy link
Member

Choose a reason for hiding this comment

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

@davidtwco Is this still true? How is the rustc rustdoc built?

Copy link
Member

Choose a reason for hiding this comment

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

Compiler documentation is not built by default - there's a config.toml flag for that. But, when enabled, compiler documentation does include internal items.

Copy link
Member

Choose a reason for hiding this comment

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

@rajcspsg Perhaps we can replace the first paragraph in this section (and the cp config.toml.example ... snippet) with what davidtwco wrote in the comment directly above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is done

@rajcspsg rajcspsg force-pushed the issue-130 branch 2 times, most recently from 19bbd6e to 9963b90 Compare September 18, 2018 21:57
@rajcspsg
Copy link
Contributor Author

@mark-i-m - I've incorporated the comments except below ones.

https://github.com/rust-lang-nursery/rustc-guide/pull/195/files#r216517973
https://github.com/rust-lang-nursery/rustc-guide/pull/195/files#r216518863
#195 (comment)
#195 (comment)

I need clarifications for your 2 comments and I have replied to your comments accordingly.
Please review and let me know if you have more comments.

@rajcspsg rajcspsg force-pushed the issue-130 branch 6 times, most recently from 4b292c1 to 2b0fac8 Compare September 24, 2018 23:57
@rajcspsg
Copy link
Contributor Author

@mark-i-m please let me know your thoughts on pending items I mentioned above.

@@ -47,6 +47,14 @@ debuginfo-lines = true
use-jemalloc = false
```

### what is x.py?
Copy link
Member

Choose a reason for hiding this comment

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

Nit: "what" -> "What"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done.

-h, --help print this help message
```

One thing to keep in mind is that `rustc` is a _bootstrapping_ compiler. That
Copy link
Member

@mark-i-m mark-i-m Sep 25, 2018

Choose a reason for hiding this comment

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

This is already discussed in the previous section. I think we don't need lines added 114-136.

EDIT: i.e. from "One thing to keep in mind" to "unless something has broken".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done.

@@ -93,3 +93,24 @@ This is much faster, but doesn't always work. For example, some tests
include directives that specify specific compiler flags, or which rely
on other crates, and they may not run the same without those options.

### Run only the tidy script
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps move these to the end of the "Running a subset of the test suites" section above (in the same file).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done.

@mark-i-m
Copy link
Member

@rajcspsg Thanks! Sorry for the delay. I have been a bit swamped lately... I left some comments (and I think I answered your questions; let me know if I haven't). Overall, this is looking good :)

@rajcspsg
Copy link
Contributor Author

@mark-i-m I have updated with your feedback. Please check and let me know if any changes are required.


Note: If you are testing out a modification to a compiler, you might want to use it to compile some project.
Usually, you do not want to use ./x.py install for testing.
Rather, you should create a toolchain as discussed in how-to-build-and-run.html#creating-a-rustup-toolchain.
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this a link to the actual chapter please?

Note: If you are testing out a modification to a compiler, you might want to use it to compile some project.
Usually, you do not want to use ./x.py install for testing.
Rather, you should create a toolchain as discussed in how-to-build-and-run.html#creating-a-rustup-toolchain.
For example, if the toolchain you created is called foo, you would then invoke it with rustc +foo ... (where ... represents the rest of the arguments).
Copy link
Member

Choose a reason for hiding this comment

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

Could you put the rustc +foo ... part in backticks so it formats as monospace font?

@mark-i-m
Copy link
Member

@rajcspsg Looking good! To minor nits, then r=me 👍

Nice job :)

@rajcspsg rajcspsg force-pushed the issue-130 branch 3 times, most recently from 85cae14 to a895d37 Compare September 29, 2018 03:14
the rest of the arguments).

[create-rustup-toolchain]: https://github.com/rust-lang-nursery/rustc-guide/blob/master/src/how-to-build-and-run.md#creating-a-rustup-toolchain
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I should have been more clear. This should be ./how-to-build-and-run.md#creating-a-rustup-toolchain

Copy link
Contributor Author

@rajcspsg rajcspsg Sep 29, 2018

Choose a reason for hiding this comment

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

Does the below one work?
[create-rustup-toolchain]: ./how-to-build-and-run.md#creating-a-rustup-toolchain

@mark-i-m mark-i-m merged commit ceff08f into rust-lang:master Sep 29, 2018
@mark-i-m
Copy link
Member

👍 Thanks!

@rajcspsg
Copy link
Contributor Author

@mark-i-m does this PR covers the 4th point in this issue?

@rajcspsg rajcspsg deleted the issue-130 branch September 30, 2018 02:59
@rajcspsg rajcspsg mentioned this pull request Oct 5, 2018
4 tasks
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