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 git branch guidelines, update build instructions in README #99

Merged
merged 5 commits into from
Jan 31, 2024

Conversation

dan-da
Copy link
Collaborator

@dan-da dan-da commented Jan 26, 2024

For consideration:

Let's consider this a first draft/proposal, and iterate from there. The goal is to reach a written policy every team member can agree with and be happy with.


Adds written policy/guidelines for branch usage and updates the README to provide instructions for building the latest release branch or the tip.

The branch guidelines are an attempt to balance/integrate/codify several things:

  1. standard github flow branching and PRs that most github devs are familiar with.
  2. simplicity for devs, ie no need to commit changes to multiple branches in normal flow and also for now we allow direct commits to master by team members without review.
  3. CI to ensure development tip of all triton/neptune crates always builds and passes tests.
  4. specifies release branches for incorporating hotfixes to past releases if necessary/desired.
  5. guidelines for keeping crate tips building against each other in between crates.io releases.
  6. provide simple instructions for end users to install the latest tagged release and thereby participate in the latest testnet/alphanet (without being too verbose).

I also threw in some recommendations for using conventional commit style and for naming topic branches. As with everything, this is a proposal, so let me know if anyone is not happy with it.

Read the file git_branches.md for details.

I tried to take all the feedback I received into account, but please let me know of any concerns or omissions.

Adds written policy/guidelines for branch usage and updates the
README to provide instructions for building the latest release branch
or the tip.
Fleshes out the guidelines with:
 * recommmendation to use conventional commit
 * a sample .gitmessage template for conv commits
 * recommendations for naming of topic branches
Copy link
Contributor

@aszepieniec aszepieniec left a comment

Choose a reason for hiding this comment

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

Thanks for the writeup.

One thing I feel rather strongly about is to minimize the friction for users who just want to participate in whatever network is currently live. To this end, I would suggest dedicating branch "release" so that it always points to something that will synchronize with the currently running network. Right now "release" agrees with "v0.0.5" whereas in the future it might agree with "v0.0.6" instead.

Some changes are backwards compatible, and some changes are not. The ones that are not backwards-compatible require a new version of testnet, and we can absorb them into "master". But it would be nice if the changes that are backwards compatible were absorbed into "release" in the mean time.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
dan-da and others added 2 commits January 26, 2024 13:37
Co-authored-by: aszepieniec <[email protected]>
Co-authored-by: aszepieniec <[email protected]>
@dan-da
Copy link
Collaborator Author

dan-da commented Jan 27, 2024

One thing I feel rather strongly about is to minimize the friction for users who just want to participate in whatever network is currently live. To this end, I would suggest dedicating branch "release" so that it always points to something that will synchronize with the currently running network. Right now "release" agrees with "v0.0.5" whereas in the future it might agree with "v0.0.6" instead.

I agree that minimizing user friction/difficulty should be a top priority. So then, we are just discussing how to achieve that.

I don't see any friction in clear installation instructions that tell the user how to build/install the present stable release. User follows instructions and is then participating in whatever network is currently live. And the only maintenance on our end is to update 2 lines in the README when publishing a new release, running git tag + git branch, and perhaps an occasional hotfix. So, I would first ask: what is the problem with doing it that way?

edit: Thinking about this more, the only opportunity I see for reduction of user friction is that a power user can stay on the same release branch, and just issue git pull, vs checking out a new tag git fetch; git checkout <release_branch>. As I see it, such a person is already able to use git. Even the git pull is already outside of our install instructions. For a user that just follows our install instructions, there is no real difference between the two approaches.

That said, I will address your points in more detail.


iiuc, you are suggesting a long-lived branch release that gets all the changes from master merged into it at release time. It further receives periodic patches for features that do not break consensus.

That is possible of course, but it may require significant work. Recall that we are talking about multiple repos, each potentially with such a release branch: neptune-core, tasm-lib, triton-vm, twenty-first. So 4 repos in total at present that must build against eachother. Someone may wish to take on the work of maintaining a release branch in each repo, but I will not be volunteering.

Some changes are backwards compatible, and some changes are not. The ones that are not backwards-compatible require a new version of testnet, and we can absorb them into "master". But it would be nice if the changes that are backwards compatible were absorbed into "release" in the mean time.

"nice", sure. but again that is extra work for somebody (or everybody) to do. And it requires splitting out non-consensus changes from consensus, which then means more testing to do as well. Probably CI should be setup for such a branch, in addition to master.


bottom line: I appreciate the feedback and will add this release branch concept into the document if you continue to push for it, or (better) you can add it with your preferred wording. However my experiences with many teams and projects recommend against that; I see more cost than benefit, given that simple instructions in the README can get users up and running on the current release/testnet, without maintaining said release branch in each repo.

@aszepieniec
Copy link
Contributor

the only maintenance on our end is to update 2 lines in the README when publishing a new release, running git tag + git branch, and perhaps an occasional hotfix

Is there a bullet point list documenting the procedure for publishing new releases? Should we have one?

I don't see any friction in clear installation instructions that tell the user how to build/install the present stable release.

Right now the relevant instructions are:

note: We recommend installing the latest release unless you are a developer intending to contribute code.

and

- for dev(unstable) skip this step. else for latest release: `git checkout v0.0.5`.

These instructions presuppose contextual knowledge that I don't think everyone has, even if they are capable of using git and capable of contributing code. For instance:

  • If you git clone you get the master branch by default.
  • The latest release lives on a branch named after its version.
  • The master branch does not correspond with the latest release.
  • "dev(unstable)" describes a branch except the branch is called master.
  • "dev(unstable)" sounds scary, so how about I just skip this step? The instruction says I can.

I would suggest

  • create a branch called release
  • drop the first line (the one that starts with "note:")
  • modify the last bullet point to say: "- git checkout release to get into the release branch, which corresponds to the network that's currently live. (Alternatively, skip this step to stay on master, which is the unstable development branch.)"

iiuc, you are suggesting a long-lived branch release that gets all the changes from master merged into it at release time. It further receives periodic patches for features that do not break consensus.

That is possible of course, but it may require significant work.

My suggestion stops at the point where the workload increases. I'm observing that a lot of changes to neptune-core are orthogonal, such as better data structures for consensus logic versus better database handling. As long as rebasing the breaking changes on top of the non-breaking ones is easy, we can update release with next to no effort. As soon as it becomes non-trivial, we freeze release until the next version is ready.

Recall that we are talking about multiple repos, each potentially with such a release branch: neptune-core, tasm-lib, triton-vm, twenty-first.

I'd say the suggestion pertains only to neptune-core and release must build against versions of crates living on crates.io.

@dan-da
Copy link
Collaborator Author

dan-da commented Jan 29, 2024

edit: @aszepieniec Can you please make edits with your preferred wording?

Ok, I think it best that I hand this off to you @aszepieniec so you can finish up the wording (and branch diagram?) as best fits your vision.

@aszepieniec
Copy link
Contributor

image

Well it looks like I pushed into your repo. Sorry about that. I did not realize that was possible.

Regardless, it does make vetting my suggested changes easier.

@aszepieniec aszepieniec merged commit b6b3b31 into Neptune-Crypto:master Jan 31, 2024
3 checks passed
@dan-da
Copy link
Collaborator Author

dan-da commented Feb 12, 2024

Well it looks like I pushed into your repo. Sorry about that. I did not realize that was possible.

oh yeah, no problem. When I make a PR from a forked repo, github provides an option to allow edits by maintainers. It defaults to yes, and I always leave it on.

So, it's a feature. ;-)

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.

2 participants