Add a CONTRIBUTING.md #1309

CleanCut · 2021-01-25T00:00:28Z

(Updated)

Added a .github/CONTRIBUTING.md file
- For the benefits of the GitHub integration
- That points to the actual contributing documentation on the website
Added a tools/ci helper script for folks to optionally use

alice-i-cecile

Thanks for making this! I'll let other folks with more experience with Bevy's CI process critique the details though :)

cart

The plan so far has been to make the Bevy Book our one stop shop for information like this. That would probably mean:

Moving this content to the book: https://bevyengine.org/learn/book/contributing/code/
Adding a link in this repo to the Contributing section of the book, either in the readme.md or to a contributing.md. I like keeping the number of files in the repo root "small", so my preference here is to just adding a Contributing section to the readme

(But I'm down to discuss the overall approach if anyone has differing opinions)

CONTRIBUTING.md

cart · 2021-01-25T03:01:29Z

Ah I just realized github has nice integrations with contributing.md. So my new preference for (2) is to add a contributing section to the readme (which directly links to the book) and add a contributing.md to the .github folder, which also links to the book.

CleanCut · 2021-01-25T05:05:25Z

I will move this content to the book and update this PR to be pointers to it, then.

We also currently use cargo clippy --all-targets --all-features -- -D warnings -A clippy::type_complexity -A clippy::manual-strip in CI.

Question: How shall we handle the fact that that command fails on master with recent nightlies? CI is pinned to nightly-2020-12-07, so we should probably either unpin the version in CI or instruct contributors to install that specific nightly and update the command above to select that specific nightly. Or stop using nightly. 😝

cart · 2021-01-25T05:42:01Z

Tough call. Moving to stable for clippy might increase build times (provided we stay on nightly for rustfmt ... idk if they share any build artifacts). More importantly, using nightly (both for clippy and fmt) really does make it harder on contributors and on CI maintenance (because of the constant rule breakage).

I really don't want to give up import merges as this will always rub me the wrong way:

// good
use bevy_asset::{AssetEvent, AssetLoader, Assets, Handle, LoadContext, LoadedAsset};

// bad
use bevy_asset::AssetEvent;
use bevy_asset::AssetLoader;
use bevy_asset::Assets;
use bevy_asset::Handle;
use bevy_asset::LoadContext;
use bevy_asset::LoadedAsset;

Why can't the world just do what I (specifically) want all of the time 😄

Maybe the pragmatic solution is to just run nightly fmt periodically to merge imports, then use stable clippy / fmt for CI.

CleanCut · 2021-01-25T06:10:09Z

I pushed a commit:

Clippy's "documentation" about config files was terrible, so I wrote a tools/ci script that might be nearly as useful.
The website pull request is in Fill out the 'Contributing Code' page. bevy-website#85
I've left the commands as currently specified in CI for the moment, pending more discussion

Why can't the world just do what I (specifically) want all of the time 😄

Seriously! It'd be so much simpler. 🤣

Tough call. Moving to stable for clippy might increase build times (provided we stay on nightly for rustfmt ... idk if they share any build artifacts).

Right now your "clean" nightly linter is a dependency for your parallel builds (for both stable and nightly). That saves you CPU time whenever "clean" fails, but wastes both wall clock time and cpu time whenever it succeeds.

I'd recommend folding the "clean" steps into one of the stable builds. Given that you've only got 10 runners under the free plan (unless you've set up self-hosted runners or paid for an enterprise plan), this will increase the odds that the "clean" stage takes longer to cut off the overall build as fast as if it were one of two root builds, but your success cases should be a couple minutes faster (as long as we can fmt, clippy, and build without cargo cleaning too much) if you've got enough runners available to run the remaining 7 jobs in parallel. On balance, it's probably worth it.

Maybe the pragmatic solution is to just run nightly fmt periodically to merge imports, then use stable clippy / fmt for CI.

I think that's a good idea. Eventually import merges will stabilize and the only behavior change you'll need to make is you'll no longer need to occasionally run nightly fmt.

DJMcNab · 2021-01-25T09:33:43Z

tools/ci

@@ -0,0 +1,20 @@
+#!/usr/bin/env bash


To ensure cross-platform compatability, this should probably be a rust-script

i.e. using the xtask pattern

yeah that does seem like a reasonable approach / would allow ci and contributors to use the exact same command on all platforms.

https://github.com/matklad/cargo-xtask

The only downsides that i can see:

more boilerplate (the consistency is worth it imo)

requires adding a default .cargo/config file, which will conflict with our recommendation to rename .cargo/config_fast_builds to .cargo/config.

xtask is a big enough change that i think its probably worth doing in a separate pr

cart · 2021-01-25T21:04:46Z

Right now your "clean" nightly linter is a dependency for your parallel builds (for both stable and nightly). That saves you CPU time whenever "clean" fails, but wastes both wall clock time and cpu time whenever it succeeds.

I'm open to these changes!

README.md

CleanCut · 2021-01-26T05:39:20Z

Right now your "clean" nightly linter is a dependency for your parallel builds (for both stable and nightly). That saves you CPU time whenever "clean" fails, but wastes both wall clock time and cpu time whenever it succeeds.

I'm open to these changes!

✅ #1318

CleanCut · 2021-01-26T05:42:10Z

@cart How are you feeling about merging this PR?

To Do in Separate PR's:

Refactor tools/ci into a rust-script using the xtask pattern [Merged by Bors] - Xtask CI #1387
Update the contributing web page with the "stable" CI commands /xref Fill out the 'Contributing Code' page. bevy-website#85, resolved in Update 'cargo fmt' command to match updated ci bevy-website#86

DJMcNab · 2021-01-26T08:15:59Z

While we're here, could we run the markdown files through a formatter, I.e. prettier or similar which word wraps them.

CleanCut · 2021-01-26T16:31:39Z

@DJMcNab Whoah, hold on there, soldier! 😄 I barely touched markdown here. If we want a certain styling of markdown files, I suggest that be discussed elsewhere and then be implemented in CI, taking into account markdown differences between platforms (website vs GitHub, primarily). Then we can make sure individual PR's adhere to it. There's no way I want to open that can of worms in this PR.

Case in point: With GitHub's markdown rendering
line breaks
might have more
effect
than you would expect.

Case in point: With GitHub's markdown rendering
line breaks
might have more
effect
than you would expect.

DJMcNab · 2021-01-26T16:55:15Z

I don't think that applies to markdown in the actually committed files. I've never seen prettier change the actual output.

I'm telling you that I asked for it here because I tried to review this commit on the mobile app, and it was completely unreadable. I have had wrap lines on in the past, but I found it was more annoying than not when the bits going over the edge of the screen were only small, creating many useless lines (which is normally the case in rustfmt formatted code with the right nightly options to deal with comments)

I did also just mean give it a single pass over with an automatic formatter, since I haven't seen a markdown formatter in rust.

CleanCut · 2021-01-26T17:53:18Z

🤷🏼‍♂️ @cart what do you want here? I think markdown linting would be better added in its own PR, but I'll do it here if that's what you want.

DJMcNab · 2021-01-26T17:57:01Z

Oh yeah, I thought I'd said that in its own PR makes sense, but that must have gotten lost in the drafting

cart · 2021-01-26T20:05:04Z

Yeah lets leave markdown formatting to a separate pr. I think this is good to merge.

This PR is easiest to review commit by commit. Followup on #1309 (comment) - [x] Switch from a bash script to an xtask rust workspace member. - Results in ~30s longer CI due to compilation of the xtask itself - Enables Bevy contributors on any platform to run `cargo ci` to run linting -- if the default available Rust is the same version as on CI, then the command should give an identical result. - [x] Use the xtask from official CI so there's only one place to update. - [x] Bonus: Run clippy on the _entire_ workspace (existing CI setup was missing the `--workspace` flag - [x] Clean up newly-exposed clippy errors ~#1388 builds on this to clean up newly discovered clippy errors -- I thought it might be nicer as a separate PR.~ Nope, merged it into this one so CI would pass. Co-authored-by: Carter Anderson <mcanders1@gmail.com>

@Ratysz

Silence those [annoying rustfmt config warnings](https://github.com/bevyengine/bevy/pull/1499/checks?check_run_id=1950282111#step:5:66) that happen because we have unstable rustfmt options in `rustfmt.toml`, but we run it in stable on CI. Thanks to @Ratysz for [calling it out](#1499 (comment)). 😄 The final approach we settled on was to comment out the unstable options in `rustfmt.toml`. Those who are using `nightly` may uncomment the unstable options locally if they wish. Once the options stabilize, we can uncomment them again. We also decided that instead of fixing the alias, we would remove the alias entirely so that we do not introduce a custom `.cargo/config.toml` that would conflict with users' custom version of the same file. This means that instead of using a `cargo ci` alias you should use `cargo run -p ci` or `cargo run --package ci` instead. <details><summary>Original Approach (abandoned)</summary> <p> _We decided **not** to go this way..._ In my quest to find a portable way to filter out the warnings I switched the library used to execute commands from `xshell` to `duct` (as advised by the `xshell` project itself when you want to do less simple things). This still uses the "xtask" pattern of using a cargo command alias and a rust project for what would have usually been done with a bash script (on posix), just a different helper library is being used internally. NOTE 1: Also, thanks to some sleuthing by @DJMcNab we were able to fix the broken cargo alias. The issue turned out to be that `.cargo/config.toml` was being ignored because of `.gitignore`. NOTE 2: This is a [known breaking change](#1309 (comment)) for anyone working on bevy who has their own local `.cargo/config.toml`. </p> </details>

@cart

[**RENDERED**](https://github.com/alice-i-cecile/bevy/blob/better-contributing/CONTRIBUTING.md) Improves #910. As discussed in #1309, we'll need to synchronize content between this and the Bevy website in some way (and clean up the .github file perhaps?). I think doing it as a root-directory file is nicer for discovery, but that's a conversation I'm interested in having. This document is intended to be helpful to beginners to open source and Bevy, and captures what I've learned about our informal practices and values. Reviewers: I'm particularly interested in: - opinions on the items **What we're trying to build**, where I discuss some of the project's high-level values and goals - more relevant details on the `bevy` subcrates for **Getting oriented** - useful tricks and best practices that I missed - better guidance on how to contribute to the Bevy book from @cart <3

CleanCut mentioned this pull request Jan 25, 2021

Create CONTRIBUTING.md guidelines #1303

Closed

alice-i-cecile reviewed Jan 25, 2021

View reviewed changes

Add a CONTRIBUTING.md

3f35d98

cart reviewed Jan 25, 2021

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

CleanCut mentioned this pull request Jan 25, 2021

Fill out the 'Contributing Code' page. bevyengine/bevy-website#85

Merged

Point to book. Add tools/ci script.

859793f

DJMcNab reviewed Jan 25, 2021

View reviewed changes

cart reviewed Jan 25, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update contributing link to point to book

2099808

CleanCut mentioned this pull request Jan 26, 2021

CI Tuning #1318

Merged

6 tasks

cart merged commit 940bcf9 into bevyengine:master Jan 26, 2021

This was referenced Feb 3, 2021

[Merged by Bors] - Xtask CI #1387

Closed

Fix errors exposed by cargo ci on nightly #1388

Closed

CleanCut mentioned this pull request Feb 24, 2021

[Merged by Bors] - Silence annoying rustfmt config warnings #1508

Closed

alice-i-cecile mentioned this pull request Mar 9, 2021

[Merged by Bors] - Adds rustfmt configs to wrap and limit comment width #1603

Closed

alice-i-cecile mentioned this pull request Apr 29, 2021

[Merged by Bors] - Comprehensive CONTRIBUTING.md #2040

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a CONTRIBUTING.md #1309

Add a CONTRIBUTING.md #1309

CleanCut commented Jan 25, 2021 •

edited

Loading

alice-i-cecile left a comment

cart left a comment

cart commented Jan 25, 2021

CleanCut commented Jan 25, 2021

cart commented Jan 25, 2021 •

edited

Loading

CleanCut commented Jan 25, 2021 •

edited

Loading

DJMcNab Jan 25, 2021

cart Jan 25, 2021

cart Jan 25, 2021

cart commented Jan 25, 2021

CleanCut commented Jan 26, 2021

CleanCut commented Jan 26, 2021 •

edited

Loading

DJMcNab commented Jan 26, 2021

CleanCut commented Jan 26, 2021 •

edited

Loading

DJMcNab commented Jan 26, 2021

CleanCut commented Jan 26, 2021

DJMcNab commented Jan 26, 2021 •

edited

Loading

cart commented Jan 26, 2021

Add a CONTRIBUTING.md #1309

Add a CONTRIBUTING.md #1309

Conversation

CleanCut commented Jan 25, 2021 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

cart left a comment

Choose a reason for hiding this comment

cart commented Jan 25, 2021

CleanCut commented Jan 25, 2021

cart commented Jan 25, 2021 • edited Loading

CleanCut commented Jan 25, 2021 • edited Loading

DJMcNab Jan 25, 2021

Choose a reason for hiding this comment

cart Jan 25, 2021

Choose a reason for hiding this comment

cart Jan 25, 2021

Choose a reason for hiding this comment

cart commented Jan 25, 2021

CleanCut commented Jan 26, 2021

CleanCut commented Jan 26, 2021 • edited Loading

DJMcNab commented Jan 26, 2021

CleanCut commented Jan 26, 2021 • edited Loading

DJMcNab commented Jan 26, 2021

CleanCut commented Jan 26, 2021

DJMcNab commented Jan 26, 2021 • edited Loading

cart commented Jan 26, 2021

CleanCut commented Jan 25, 2021 •

edited

Loading

cart commented Jan 25, 2021 •

edited

Loading

CleanCut commented Jan 25, 2021 •

edited

Loading

CleanCut commented Jan 26, 2021 •

edited

Loading

CleanCut commented Jan 26, 2021 •

edited

Loading

DJMcNab commented Jan 26, 2021 •

edited

Loading