-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introducing DEVELOPMENT.md #5209
Conversation
I think you might have accidentally included some commits from another branch. Could you remove those? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I think as general feedback I want this to be a bit more concise where possible or at least structure it in a way where people can get started quickly.
Maybe we could also use tricks like the HTML <details>
element to hide platform-specific instructions?
I'll also remove the link to #5195 because this won't be enough to close that issue.
DEVELOPMENT.md
Outdated
### GNU utils and prerequisites | ||
If you are developing on Linux, most likely you already have all/most GNU utilities and prerequisites installed. | ||
To make sure, please check GNU coreutils [README-prereq](https:/coreutils/coreutils/blob/master/README-prereq) | ||
|
||
**Tip:On MacOS** you will need to install [Homebrew](https://docs.brew.sh/Installation) and use it to install the following formulas: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be moved down because it's only important when running the GNU test suite (unless there are also some things you need for regular development).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few things that are needed outside of GNU tests, namely pre-commit
(even though it is currently broken on MacOS). Also Homebrew seems to be inevitable for development environment on MacOS anyway, and especially so for this project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But tools like pre-commit also have their own section with installation instructions already. For some others I don't understand what they are used for (like wget & texinfo are those used by GNU?). So maybe we could also add some comments about which packages are used for what.
I do like providing an easy list like this, but maybe that could go at the end and you can link to it here? I think it's quite important that the start of the document is relevant for everyone. What do you think of a structure like this?
## Before you start
## Tools
[All tools and cross-platform stuff here]
## Tips for installing on Mac
## Tips for installing on Windows
I appreciate you dedication to helping newcomers btw. It's also good to have someone with a mac perspective around. I had no idea about all the extra stuff that was needed 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. and yes, most of the Homebrew formulas are needed by GNU build/tests - i just ran it over and over, fixing failures one by one and that is the final list.
Will update with structure you suggested
DEVELOPMENT.md
Outdated
**Tip:On MacOS** you will need to install C compiler & linker: | ||
`xcode-select --install` | ||
|
||
**Tip:On Windows** On Windows you'll need the MSVC build tools for Visual Studio 2013 or later. | ||
To acquire the build tools, you’ll need to install Visual Studio 2022. When asked which workloads to install, include: | ||
- Desktop Development with C++ | ||
- The Windows 10 or 11 SDK | ||
- The English language pack component, along with any other language pack of your choosing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to mention all of this? If it's a very common pitfall for newcomers I'm all for including it, but it feels maybe a bit too detailed? All bits of information here also need to be kept up to date (i.e. when people should install Visual Studio 2023 instead of 2022). So, just saying "install rust via rustup" should probably be fine. We could also link to more extensive installation guides instead.
This is how simple it could be: https:/nushell/nushell/blob/main/CONTRIBUTING.md#setup (though we could add a bit more info if we really want to).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, could be simplified. On the Nu shell example though - they offload all details to linked https://www.nushell.sh/book/installation.html for installing Rust toolchain and all dependencies/prerequisites - that makes their CONTRIBUTING.md clean and simple. Woulkd the equivalent in our case be moving all those details out of CONTRIBUTING.md to DEVELOPMENT.md ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah I guess you're right. We can keep it and just trim it a little bit.
DEVELOPMENT.md
Outdated
|
||
## Code coverage | ||
|
||
<!-- spell-checker:ignore (flags) Ccodegen Coverflow Cpanic Zinstrument Zpanic --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this to the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to the top of the Testing section or somewhere else?
DEVELOPMENT.md
Outdated
|
||
## Before you start | ||
|
||
For this guide we assume that you already have GitHub account and have `git` and your favorite code editor or IDE installed and configured to work with GitHub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configured to work with GitHub.
Nit, but the IDE doesn't need GitHub integration.
DEVELOPMENT.md
Outdated
For this guide we assume that you already have GitHub account and have `git` and your favorite code editor or IDE installed and configured to work with GitHub. | ||
Before you start working on coreutils, please follow these steps: | ||
|
||
1. Fork [coreutils repository](https:/uutils/coreutils) to your GitHub account [ref:](https://docs.github.com/en/get-started/quickstart/fork-a-repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Fork [coreutils repository](https:/uutils/coreutils) to your GitHub account [ref:](https://docs.github.com/en/get-started/quickstart/fork-a-repo) | |
1. Fork the [coreutils repository](https:/uutils/coreutils) to your GitHub account [ref:](https://docs.github.com/en/get-started/quickstart/fork-a-repo) |
I'd also like to have some more text as the link than "ref". Maybe See [this GitHub guide](...) for more information.
GNU testsuite comparison:
|
made changes to DEVELOPMENT.md and simplified CONTRIBUTING.md (offload and link all dev details to DEVELOPMENT.md). There are some TODOs left - looking for feedback |
DEVELOPMENT.md
Outdated
@@ -0,0 +1,331 @@ | |||
<!-- spell-checker:ignore reimplementing toybox RUNTEST CARGOFLAGS nextest prereq autopoint gettext texinfo automake findutils shellenv libexec gnubin --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- spell-checker:ignore reimplementing toybox RUNTEST CARGOFLAGS nextest prereq autopoint gettext texinfo automake findutils shellenv libexec gnubin --> | |
<!-- spell-checker:ignore reimplementing toybox RUNTEST CARGOFLAGS nextest prereq autopoint gettext texinfo automake findutils shellenv libexec gnubin toolchains --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! If you do not mind me asking - what are you using for spell checker? The VSCode spellchecker extension did not catch this, even though it is enabled for markdown files by default
Co-authored-by: Sylvestre Ledru <[email protected]>
please let me know when it is ready to be reviewed :) |
yep, should be ready |
GNU testsuite comparison:
|
you know that merging isn't necessary ? ;) |
CONTRIBUTING.md
Outdated
<!-- spell-checker:ignore (flags) Ccodegen Coverflow Cpanic Zinstrument Zpanic --> | ||
|
||
Code coverage report can be generated using [grcov](https:/mozilla/grcov). | ||
## TODO : Any guidelines for branching, PRs, etc? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove it, we don't do it yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies - have been traveling. will do
CONTRIBUTING.md
Outdated
|
||
If you are using stable version of Rust that doesn't enable code coverage instrumentation by default | ||
then add `-Z-Zinstrument-coverage` flag to `RUSTFLAGS` env variable specified above. | ||
***TODO*** The technical details on how to generate report locally are offloaded to DEVELOPMENT.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please fix this one too
thanks, will keep that in mind in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very good! The content is great, I just have few comments on the formatting.
DEVELOPMENT.md
Outdated
|
||
If you're using rustup to install and manage your Rust toolchains, `clippy` and `rustfmt` are usually already installed. If you are using one of the alternative methods, please make sure to install them manually. See following sub-sections for their usage: [clippy](#clippy) [rustfmt](#rustfmt) | ||
|
||
***Tip*** You might also need to add 'llvm-tools': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
***Tip*** You might also need to add 'llvm-tools': | |
***Tip***: You might also need to add 'llvm-tools': |
It might also be nice to mention what those tools are for. Is it code cov?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, will update
DEVELOPMENT.md
Outdated
Before you start working on coreutils, please follow these steps: | ||
|
||
1. Fork the [coreutils repository](https:/uutils/coreutils) to your GitHub account. | ||
***Tip:*** See [this GitHub guide](https://docs.github.com/en/get-started/quickstart/fork-a-repo) for more information on this step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nitpicky, but there's a few periods missing at the end of sentences throughout this doc.
Co-authored-by: Terts Diepraam <[email protected]>
DEVELOPMENT.md
Outdated
On MacOS you will need to install [Homebrew](https://docs.brew.sh/Installation) and use it to install the following Homebrew formulas: | ||
|
||
```shell | ||
brew install coreutils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it cannot be done on a single line ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it sure can, but how about this version instead?
brew install \
coreutils \
autoconf \
gettext \
wget \
texinfo \
xz \
automake \
gnu-sed \
m4 \
bison \
pre-commit \
findutils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ship it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this! I'll let Sylvestre make the final merge, but it's got my approval. Thanks!
Adding DEVELOPMENT.md with more detailed instruction on setting local development environment