avr-rust / delay

arduino-like delay routines based on busy-wait loops
Apache License 2.0
15 stars 11 forks source link

Set reasonable defaults for cargo config #26

Closed n8henrie closed 2 years ago

n8henrie commented 2 years ago

This sets build-std=core by default and uses the atmega328p as the default target. Users will still need to specify alternate targets if desired, but it makes the build process much easier for a default case.

In a similar vein as https://github.com/avr-rust/delay/pull/25 and https://github.com/avr-rust/delay/pull/24, this makes it very simple for a very common case (the default used in the README and the testing docker container); if all of these are merged, users building for atmega328p should be able to just git clone and cargo build --release and be on their way without need for additional flags such as +nightly or build-std=core or --target...

stappersg commented 2 years ago

The commit message should not have any references the web where the git repository is hosted.

So remove those references. Yeah, I did that on the previous two merge requests from @n8henrie and I done with keep doing that. github.com is nice packaging material (wrapping paper, bubble plastic) that has no reason to be present in all the cloned git repositories, the actual product we are working on.

stappersg commented 2 years ago

The commit message should not have any references to the web server where the git repository is hosted.

So remove those references. Yeah, I did that on the previous two merge requests from @n8henrie and I done with keep doing that. github.com is nice packaging material (wrapping paper, bubble plastic) that has no reason to be present in all the cloned git repositories, the actual product we are working on.

n8henrie commented 2 years ago

No problem -- your house, your rules. I do think it's worth clarifying a few ideas though.

The commit message should not have any references to the web server where the git repository is hosted.

It doesn't -- I think you might be confusing the body of the pull request with the commit message, which I was trying to disambiguate here.

You're right that the commit message will end up in the git repo and log. However, the body of the pull request is a GitHub-specific feature; as far as I know, pull / merge requests aren't implemented by git itself.

My commit message doesn't reference github. You can prove this to yourself like so:

$ # clone the branch that this pull request originates from
$ git clone --branch=set-cargo-config --depth=1 https://github.com/n8henrie/avr-rust-delay.git
$ cd ./avr-rust-delay
$ git log --author='Nathan Henrie' | grep -i github
$ git log --author='Nathan Henrie'
commit 2f0f679c22212933791f298f39f8c1fb450afabb (grafted, HEAD -> set-cargo-config, origin/set-cargo-config)
Author: Nathan Henrie <nate@n8henrie.com>
Date:   Sat Jun 11 06:18:05 2022 -0600

    Set reasonable defaults for cargo config

    This sets `build-std=core` by default and uses the atmega328p as the
    default target. Users will still need to specify alternate targets if
    desired, but it makes the build process much easier for a default case.

Having seen your strong feelings on commit messages, I tried to do accordingly with mine.

Yeah, I did that on the previous two merge requests from @n8henrie and I done with keep doing that.

I can see your edits on my prior two PRs, and I'm very confused as to your motivations. You're editing out reference to GitHub in comments that are only accessible through GitHub. EDIT: In other words, my text above is similar to your linking to other issues here: https://github.com/avr-rust/ruduino/issues/46#issue-1264731826

github.com is nice packaging material (wrapping paper, bubble plastic) that has no reason to be present in all the cloned git repositories, the actual product we are working on.

Again, as far as I know none of the content you've been editing ends up in the git repositories. All you're doing is editing content hosted by GitHub that does not go into the repository. If I'm wrong about this I'd be happy to have you show me!

@shepmaster @dylanmckay -- is this your understanding as well?

Is avoiding mentions of GitHub in pull request bodies and/or commit messages a core value for the avr-rust group? I'm sorry if this is documented somewhere, and I've missed it.

Further, I'd like to point out that these links -- for me and many others -- are important and helpful points of documentation. I add them intentionally so that people researching other branches, issues, PRs, and repos can see that a relevant topic has been mentioned in another thread. As a matter of fact, that's how I found this repo in the first place, and it's how I was notified about the recent (and very exciting) updates to the nightly compiler and llvm_asm -> asm macro updates in the avr-rust repos that allow compilation for atmega328p with a current nightly! And that's not to mention the handy ability to automatically close issues upon merge with commit messages...

stappersg commented 2 years ago

The important thing is that the project moves forward.

Let see how the "This", written as

[This](This)

ends up in the git repo we care about.

stappersg commented 2 years ago

Is avoiding mentions of GitHub in pull request bodies and/or commit messages a core value for the avr-rust group? I'm sorry if this is documented somewhere, and I've missed it.

Partial output of git log, do note # 27 and # 24

commit b88b4738570be1df1390011d07cc3b7a314bc822
Merge: 5395bfe 7032d81
Author: Geert Stappers <stappers@stappers.it>
Date:   Sat Jun 11 18:30:23 2022 +0200

    Merge pull request #24 from n8henrie/vcs-toolchain

    Track toolchain in VCS

commit 5395bfe0647bfd63dc326a5a1b50e6090ee7f22b
Merge: 6ba963c 7c6d57d
Author: Geert Stappers <stappers@stappers.it>
Date:   Sat Jun 11 16:13:57 2022 +0200

    Merge pull request #27 from n8henrie/builtin-target

    Use built-in target

commit 7c6d57d9f7dd77bdc74dfb866a3ff56e825cabe8
Author: Nathan Henrie <nate@n8henrie.com>
Date:   Fri Jun 10 06:59:29 2022 -0600

    Use built-in target

commit 7032d81c5b4bee5ed4a28bc5c4c783a97ba5b7fb
Author: Nathan Henrie <nate@n8henrie.com>
Date:   Sat Jun 11 05:59:17 2022 -0600

    Pin toolchain in VCS

    This will lead to a less confusing approach for end users, as they'll be
    able to clone the repo and have the toolchain preconfigured.

The #24 and #27, above written as # 24 and # 27, do I read as _leave the code you are working on, come to us, come feed us further, we love to lure you".

stappersg commented 2 years ago

afbeelding

n8henrie commented 2 years ago

The important thing is that the project moves forward.

I agree, and I'm not trying to be a troublemaker here.

Can you find your comment above anywhere in the git repo itself? I doubt it, because I don't even know where one would look for it -- this PR is not yet merged into https://github.com/avr-rust/delay, and your comment is not anywhere to be found on my fork at https://github.com/n8henrie/avr-rust-delay, so where would it even be?

As another test, I created a test branch and test pull request at a dummy repo here: https://github.com/n8henrie/killed-9-example/pull/3

Please note the test in the pull request: This is a pull request message! It is relevant to https://github.com/avr-rust/delay/pull/26

I merged that PR. Let's see if it shows up in the git repo by searching for the word relevant:

$ git clone https://github.com/n8henrie/killed-9-example.git
$ cd killed-9-example
$ git checkout n8henrie-patch-1
$ git log --grep relevant
$ grep -rai relevant ./.git/
$

The pull request content is not found as far as I can tell.

stappersg commented 2 years ago

afbeelding

New attempt on the formatting

"leave the code you are working on, come to us, come feed us further, we love to lure you"

n8henrie commented 2 years ago

above written as # 24 and # 27

That text is from your merge commit, not coming from my pull request. You control that content, I cannot.

stappersg commented 2 years ago

Note the Merge: 29adeff 2f0f679

$ git log | head -n 16
commit 8ec7a7c6c9e6aceb85f3e713cf7dda76a728ed17
Merge: 29adeff 2f0f679
Author: Geert Stappers <stappers@stappers.it>
Date:   Sun Jun 12 20:04:26 2022 +0200

    Merge pull request #26 from n8henrie/set-cargo-config

    Set reasonable defaults for cargo config

commit 29adefff19f865ba7ee475612f4e3cffc5298e6c
Author: Geert Stappers <stappers@stappers.it>
Date:   Sun Jun 12 18:56:47 2022 +0200

    white space change by `cargo fmt`

commit b88b4738570be1df1390011d07cc3b7a314bc822

commit 29adeff is visible. for the actual commit 2f0f679:

$ git log | sed --silent -e '/^commit 2f0f679/,/^commit/p'
commit 2f0f679c22212933791f298f39f8c1fb450afabb
Author: Nathan Henrie <nate@n8henrie.com>
Date:   Sat Jun 11 06:18:05 2022 -0600

    Set reasonable defaults for cargo config

    This sets `build-std=core` by default and uses the atmega328p as the
    default target. Users will still need to specify alternate targets if
    desired, but it makes the build process much easier for a default case.

commit 7032d81c5b4bee5ed4a28bc5c4c783a97ba5b7fb

So the This became indeed This, not [This](This).

What happend to

In a similar vein as https://github.com/avr-rust/delay/pull/25 and https://github.com/avr-rust/delay/pull/24, this makes it very simple for a very common case (the default used in the README and the testing docker container); if all of these are merged, users building for atmega328p should be able to just git clone and cargo build --release and be on their way without need for additional flags such as +nightly or build-std=core or --target...

is unknown to me.

n8henrie commented 2 years ago

What happend to ... is unknown to me.

This is what I've been trying to explain above -- that text is all on GitHub as part of the pull request (a GitHub-specific feature) and not directly relevant to git or the git repo.