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

Add Chromium section #1479

Merged
merged 34 commits into from
Nov 27, 2023
Merged

Add Chromium section #1479

merged 34 commits into from
Nov 27, 2023

Conversation

adetaylor
Copy link
Collaborator

@adetaylor adetaylor commented Nov 14, 2023

This is a contribution of a Chromium section for Comprehensive Rust.

Copy link
Collaborator

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

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

This is excellent, thanks for putting this together! Content-wise this is excellent. I really the way you made the CXX exercises about exploring how CXX interop works. I will probably steal that for the Android course at some point 😉

There are a couple of issues that need to be addressed, though:

  • Many of the slides are too long. We generally try to avoid making slides so long that the instructor will need to scroll (and we've received some specific feedback indicating that scrolling doesn't read well in the presentation). You'll need to break up large slides into multiple smaller slides. You already have most of these slides broken up into multiple sections, so in general you should probably be able to make each section its own slide. Also, don't worry too much if some slides have to be a bit long. We're finding in the v2 rewrite that it's hard to always keep within that "no scrolling" limit, so it's not a hard and fast rule.
  • These slides are very text-heavy. That's generally to be expected, and I think this will be less of an issue once the larger slides have been broken up. But in general I suggest trying to be less verbose in the slides, and move any non-critical information into the speaker notes for the instructor to tell students.

Also, you put this PR up as a draft. Do you have specific changes you're still planning on making (other than addressing the review feedback)?

src/chromium/cargo.md Outdated Show resolved Hide resolved
src/chromium/interoperability-with-cpp.md Outdated Show resolved Hide resolved
src/chromium/interoperability-with-cpp.md Outdated Show resolved Hide resolved
src/chromium/interoperability-with-cpp.md Outdated Show resolved Hide resolved
src/exercises/chromium/bringing-it-together.md Outdated Show resolved Hide resolved
@adetaylor
Copy link
Collaborator Author

Thanks for the early feedback! I will certainly work on making the slides fit into a screen.

Relatedly, I tend to like diagrams/pictures. In the past I've had success with mdbook-mermaid - I may have a crack at including that, so long as the accessibility features turn out to work. Any concerns with that? There's only one or two places I'd want to add such a diagram but I think it would help break up the "walls of text" concern.

Regarding why it's a draft PR: just because I haven't yet had time to think about the optimal review process. It needs some combination of review from the Comprehensive Rust folks and also from Chromies. It's complicated by the fact that the Chromium Rust infrastructure, especially the gnrt stuff, is a moving target, so much of that section is accurate this week but will need significant rework for next week's state of the tooling.

@mgeisler
Copy link
Collaborator

Thanks for the early feedback! I will certainly work on making the slides fit into a screen.

Please see #1464 for some thoughts on the size and style of the slides for v2 (which would also apply here).

@adetaylor
Copy link
Collaborator Author

OK, I've had a go at making the slides less text-heavy, splitting them up a bit and moving more into speaker notes.

I have not yet done this for the third-party section, since I anticipate I'll have to rewrite all that next week anyway because of some tooling changes in Chromium.

But I consider the rest of it ready for review!

@adetaylor adetaylor marked this pull request as ready for review November 17, 2023 12:17
src/chromium/policy.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@anforowicz anforowicz left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great! I've left some feedback about minor details here and there.

@adetaylor
Copy link
Collaborator Author

Thanks! Looks great! I've left some feedback about minor details here and there.

Thanks very much Łukasz!

Copy link
Collaborator

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

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

This looks pretty good now! Some of the slides are still pretty wordy, but I don't think we need to completely fix that before merging. I just had one more comment that needs to be addressed before approval :)

src/chromium/interoperability-with-cpp/example-bindings.md Outdated Show resolved Hide resolved
src/chromium/cargo.md Outdated Show resolved Hide resolved
src/chromium/policy.md Outdated Show resolved Hide resolved
This is the first draft of a Chromium section for Comprehensive Rust.
@adetaylor
Copy link
Collaborator Author

This looks pretty good now! Some of the slides are still pretty wordy, but I don't think we need to completely fix that before merging. I just had one more comment that needs to be addressed before approval :)

Done!

Would you like me to squash this branch before approval?

Also, I don't have permission to kick off the CI jobs - can you confirm they pass before merging? I strongly suspect they don't yet pass.

I have now run through the class and I have some upcoming changes based on that experience, but they shouldn't prevent merging this. They can come along as a separate pull request if we happen to merge this first.

Based on feedback running the course for the first time,
* it wasn't obvious why anyone would ever want to use cargo rather than gn
* the section about how to run cargo --offline was too detailed, and has
  been moved into documentation
It was previously in the speaker notes for the wrong slide.
This also reorders pages a little.
This was unclear to the first folks who sat through the course.
This isn't really necessary now that Chromium has switched to a standard
Cargo.toml.
First run throughs of the course showed this was confusing. Cease to talk about
it as a "problem to solve" but instead highlight it as an aspect of the paths
in which crate metadata are stored.

This also splits the "generating-gn-build-rules.md" slide into two, because it
was too long.
Based on experience of teaching the course once.
@adetaylor
Copy link
Collaborator Author

OK, I pushed all the changes based on the feedback from the first time I tried to teach the course.

@anforowicz perhaps you'd like to look at the commits since the last force-push?

But in any case I think this is good to merge - let me know preferences regarding squashing.

Copy link
Collaborator

@anforowicz anforowicz left a comment

Choose a reason for hiding this comment

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

In my 2nd pass I only got as far as the beginning of the "Adding third party crates" chapter. Still, it's probably best if I flush out my comments and I can continue later (or submit a separate PR later). I will try to spend some time on error handling to tweak the examples - I have some notes that we can discuss on Monday.

src/chromium/build-rules.md Outdated Show resolved Hide resolved
src/chromium/build-rules/unsafe.md Outdated Show resolved Hide resolved
src/chromium/build-rules/vscode.md Outdated Show resolved Hide resolved
@mgeisler
Copy link
Collaborator

Would you like me to squash this branch before approval?

I've set this repository to squash-merge all PRs: we've had a lot of drive-by contributors, so I wanted to make it super easy to maintain a clean history.

In other words, don't worry about the commits, only the PR title and description will survive. It's a bit weird when you're used to maintain a proper history, but it's oh-so-liberating as a reviewer since I can just add fixups in-flight without worrying about the broken state on the branch 🙂

@mgeisler
Copy link
Collaborator

@adetaylor and @anforowicz, you've both been invited to the repository now — that should allow you to review and submit the PR independently. I consider this corner of the course yours: you'll be the ones teaching it going forward, so you get to maintain it 🙂

But please don't hesitate to let me, @randomPoison, @djmitche, etc know if you have questions or run into problems!

README.md Show resolved Hide resolved
@adetaylor
Copy link
Collaborator Author

@anforowicz if you're happy with this, please formally review it so we can press merge — we can and should continue to tweak over the coming days before the first proper run of the course next week (though I'm at an off-site event for the rest of the week so may not be super responsive)

@adetaylor adetaylor changed the title First cut of Chromium section Add Chromium section Nov 27, 2023
@adetaylor adetaylor merged commit 7f469fb into google:main Nov 27, 2023
31 checks passed
mgeisler added a commit that referenced this pull request Nov 30, 2023
This incorporates the new Chromium material from #1479 as well as the
reshuffle and rewrite from #1073.
mgeisler added a commit that referenced this pull request Nov 30, 2023
This incorporates the new Chromium material from #1479 as well as the
reshuffle and rewrite from #1073.

This is a clean update without any additional changes. I’ll un-fuzzy
the string later.
mgeisler added a commit that referenced this pull request Dec 2, 2023
This incorporates the new Chromium material from #1479 as well as the
reshuffle and rewrite from #1073.

This is a clean update without any additional changes. I’ll un-fuzzy the
string later.
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