Contributing to AgOpenGPS

Software Feature requests
1

Hi

Let me start by commending all the hard work that everyone that is part of the AgOpenGPS community and especially @BrianTee_Admin has done.

In the past few days I tried to contribute to the project with the following efforts:

  • Converting the WinForms projects to SDK-style: #426
  • Using NuGet for dependency management, instead of referencing “raw” DLLs: #432

My experience with these attempts to contribute to AgOpenGPS were slightly frustrating for the following reasons:

  1. The workflow for how to contribute to the project is not obvious at all: there seems to be a random new dev-branch every few days that must/should be used to base any new work off of.
  2. My first PR somehow broke things for @Richardklasens_admin & @BrianTee_Admin. This was of course not my intention and I asked for more information about the problems they were facing, such that I could investigate and fix them. However, I did not get any response to my questions.
  3. My second PR was merged into the dev-branch, but got reverted without any further comment a few hours later.

I absolutely understand, that maintaining an open-source project is hard work and not every contribution is can make it into the final product, but if this is how every new contributor feels, there is a big barrier for entry for anyone that has something to offer to the community and the project.

If you are open to suggestions, I would propose to either overthink the entire contribution workflow (do you really need a dev-branch?) or at least make it more obvious how to contribute “correctly”.

Best regards,
Matthias

2

Hi Matthias.
First of all, thanks for helping to contribute.
You’re PR just came in a few days before our New 6.4.0 release. It was bit of frustrating time to get it out, so how it’s being handled with your PR’s should not be the way to go. Sorry for that.

If you want to contribute, please take latest Master or if develop branch is there take that one, and merge your edit locally in a new branch called feature-nameofyoursummedchanges

If it runs fine, you can do a PR with your new branch into the repository.
Then, about your latest PR’s

We had both a lot of compile errors when trying to build your PR, so that won’t work.

The second one build ok, but when the program started the framerate when to 60/70ms per frame instead of the normal 2/3 ms per frame. So something is really bad there. @BrianTee_Admin tried it, but he had to remove his local repository and redownload it from git to continue working again. That’s why he Reverted it back, due to the patches and changes for the already published release he didn’t had time to respond. Sorry for that.

Can you explain what you want to archieve with your PR’s? What’s the benefit?
Maybe @DanielP and/or @Jozef can also look into it?
Thanks again for your contribution and willing to help AOG grow!

Richard

3

Hi Richard

Thank you for explaining the situation.

You’re PR just came in a few days before our New 6.4.0 release. It was bit of frustrating time to get it out, so how it’s being handled with your PR’s should not be the way to go. Sorry for that.

I absolutely do not expect, that a PR is being addressed immediately. In fact, this is the risk for me as a contributor, that it might take a long time until someone looks at my PR and maybe even rejects it.

If you want to contribute, please take latest Master or if develop branch is there take that one, and merge your edit locally in a new branch called feature-nameofyoursummedchanges

Yes, this is the workflow that I am expecting, but looking at the most recent PRs shows a different picture:

  • The target branch is rarely the “master” branch and I’m not sure if a “develop” branch has even been used in the past.
  • New branches are created for PRs, which are then closed. Why?
  • Changes from PRs are being added manually, instead of merged. Why?

It all just looks a bit odd to me and I don’t see why you are working this way. If there is a good reason for doing so, by all means ignore my questions and keep doing it the way it works for you.
I just wanted to point out that it is not obvious for any new contributor, what is expected of them (especially because your workflow does not match the majority of open-source projects).

We had both a lot of compile errors when trying to build your PR, so that won’t work.

That’s of course very unfortunate and I suspect that removing the obj/ and bin/ folders would have solved this issue.

The second one build ok, but when the program started the framerate when to 60/70ms per frame instead of the normal 2/3 ms per frame. So something is really bad there.

This is odd. I would love to investigate, why this is the case, but only if the changes themself are something you want to have.

Can you explain what you want to archieve with your PR’s? What’s the benefit?

This is a valid point and I agree that I should have stated more clearly in the PR what my intention was.
Overall, I was trying to modernize the codebase:

  • SDK-style projects are the first step towards migrating to a newer .NET version (like .NET 6 or 8), which is also why I was asking if there are any plans to migrate in my PR.
  • Using NuGet for dependency management would allow easier updates in the future.

If these changes do not align with your long-term goals, I totally understand if you don’t want to integrate them.

Thank you for taking your time to explain you reasoning.
Regards,
Matthias

4

Hi Matthias.

Well…

Let’s start do it the right way (I guess)
I’ve created a new branch called feature-sdkstyle.
It’s based on master which contains I believe the latest updates.

Pull it, do you merge local, commit and create a PR of it, i will merge it into the feature branch. I think that’s the way it should work right?

From there we can continue.
Have to say, @Jozef is currently working on a private repo to refactor the whole project. I can’t tell where the future for AOG is at the moment, there is also a project going on, with a QT framework to make it multi platform, so…. Pretty hard to spell the future. But doing nothing won’t help at all. I’m a guy who wants to connect every possible user/coder/contributor together and hopefully there will be a succes in the end.

If you need more then what I’ve done now, let me know!

Best regards,
Richard

5

Hi Richard

I’m afraid, I don’t think that this approach is any better (or even different) than what you are doing until now.
A contribution should not require any communication prior to the PR being created (except of course if the contribution somehow needs some preliminary clarification).

If you have to first create a branch for every contribution, this is a lot of work from your side that should not be necessary.
It also requires every contributor to be aware of this workflow: contact you in some way, wait for you to create the branch and then finally create the PR.

Additionally, if work is being done in the “master” branch, while the feature branch is not yet merged, there is the potential of having merge conflicts down the line.
But instead of the PR author being responsible for resolving those conflicts (which should be the case), you are now responsible yourself to successfully merge the feature branch back into “master”.

I would strongly recommend to rethink, if it is really necessary to have a different branch than “master” being the target branch of your PRs.

Have to say, @Jozef is currently working on a private repo to refactor the whole project. I can’t tell where the future for AOG is at the moment, there is also a project going on, with a QT framework to make it multi platform, so…. Pretty hard to spell the future.

Oh wow, interesting… the entire project being refactored in a private project sounds… let’s say: brave?
Is this a complete rewrite? If not, how do you ensure that all new changes that happen in the meantime are still part of this refactor?

I totally understand, that you cannot predict the future, but you seem to have a plan at least if there is this refactoring happening. This again is something that should be much more transparent for anyone contributing to AgOpenGPS. If I am investing time and effort into some implementation, I would be pretty disappointed if this change disappears a few days later, when the refactor “goes live” (whatever this means).

I also agree, that doing nothing will not help the situation, but I would focus my efforts on working out some kind of plan or some kind of “what if…” scenarios:

  • What if the rewrite is finished? Is the code simply replaced by the refactor?
  • What if the Qt port is mature enough to replace AgOpenGPS? Will you abandon the C# version?
  • …

Sorry if I’m being pedantic here, but I just want to give you some things to think about.

Best regards,
Matthias

2 Likes
6

Maybe we need to know why the master is a protected branch. I am just a aog user, and as I see it we use the master, and would be disappointed to download the program to my tractor tablet, and then find out aog run poorly. That is what I see could happen if I accidentally downloaded minutes after a merge with unseen problem had been done.
.

7

We only push to master if we sure the program runs fine. We don’t work from it… there is always a “dev” branch, also it’s not caring the name dev or development. Once we do a merge, it’s always to dev and from dev to master.

8

Hi matthias.

So… Best to do a create a develop branch… its up now.
From there you can continue? its up with bugfix641 branch.

For what we’ve learned with Winforms and git,
When you work on a project with multiple developers there can be some tricky parts when merging files. It’s due to the fact that when using VS and building on a winforms project, there is a lot of code generated by the application itself. Especially when you do stuff on the GUI-side of the project. Maybe because its how the project is setup, not sure about that, i’m not a well contained C# code specialist, for me its just better to manage stuff, thats what i do for living as well, so that makes sense.

I think that is why Brian is a little bit scary to merge PR’s, in the past it gaves not the best results.
So, maybe you can help us a little to start setting up a right way of how to do this in the first place.

Master is protected, good as is…it’s has to be up with the release.
Develop should be a branch in between features, user commits, and probably patches to continue development. @BrianTee_Admin should do his patches into the current bugfix_641 branch, merge to develop and wait there until its ready to push to master and create a release. In my mind that is how it should work. But maybe i’m wrong.

Aboiut the refactoring.
@Jozef is tyring to clean up the whole project, split files, create classes, and a lot more.
He’s working on say v7, but actually, that project, and i’m hate to say this, is already about 50 merge commits out of sync from our current release. Sad to think about that.

So… please, start would be, how to deal with the Git process, and not having conficts due to Winforms projects and merging, thats the biggest “banging your head thru the wall” mind for @BrianTee_Admin.

Greetz, Richard

9

Okay, I can see this being a problem. However, this is also an indicator of an other issue:

  • Merging a PR should only happen when the functionality has been verified.
    • Having a GitHub action workflow that verifies that at least the code compiles would be a first step. (Actually it seems like this has only recently been removed: 315ff94… why?)
    • Having tests would help to add confidence that core functionality is not broken. (However, in order to be able to add those, the structure of the code would need to be changed.)

I think this is the way to go, yes. This will most likely match the Git Flow branching model and should work fine for this use-case.
Two important notes:

  1. Both the “master” and the “develop” branch have an infinite lifetime, meaning that the “develop” branch does not get removed when it is merged.
  2. I would suggest to
    • either make the “develop” branch the default branch, such as to nudge new contributors towards using the correct branch
    • or document the entire contribution process somewhere (e.g. in the README) to highlight that the “develop” branch should be targeted

For what we’ve learned with Winforms and git,
When you work on a project with multiple developers there can be some tricky parts when merging files. It’s due to the fact that when using VS and building on a winforms project, there is a lot of code generated by the application itself. Especially when you do stuff on the GUI-side of the project. Maybe because its how the project is setup, not sure about that, i’m not a well contained C# code specialist, for me its just better to manage stuff, thats what i do for living as well, so that makes sense.

Yes, WinForms is probably not the easiest framework regarding multiple developers working on the same form.
However, I think that this could be remedied by possibly splitting up some of the huge forms into smaller sub-forms that could also improve readability.

So, maybe you can help us a little to start setting up a right way of how to do this in the first place.

I don’t think this can be done with simple changes. While the introduction of the SDK-style project format could help to reduce merge conflicts in the .csproj files, to reduce the conflicts in the .cs files requires more structural changes (as mentioned above).

Develop should be a branch in between features, user commits, and probably patches to continue development. @BrianTee_Admin should do his patches into the current bugfix_641 branch, merge to develop and wait there until its ready to push to master and create a release. In my mind that is how it should work. But maybe i’m wrong.

That’s it!
I’m also not opposed to @BrianTee_Admin or other maintainers being able to directly work on the “develop” branch, because opening a PR simply for being able to merge does not add any value. A PR makes sense for changes that need to be reviewed, which probably is not needed for @BrianTee_Admin.

He’s working on say v7, but actually, that project, and i’m hate to say this, is already about 50 merge commits out of sync from our current release. Sad to think about that.

That’s what I was afraid of, and it will only become worse over time.
This is why it would make sense to have a plan about the transition, even if it is not ready yet.

I hope my wall of text makes sense and helps somehow.

Regards,
Matthias

1 Like
10

I have been looking at the current state of the process on GitHub and if you are interested, here are a few remarks:

  • Merging into master should only happen from the develop branch and no other feature branch (e.g. BugFix_641).
  • master and develop should be the only long-lived branches, every other branch should be deleted after it has been merged.
  • What is this develop-nightly branch? The develop branch itself should be considered “nightly” (e.g. “unstable”).
  • I don’t think it is necessary for maintainers to create a PR for merging a feature. There is no value in that, if no code review or other collaboration is being done.

Best regards,
Matthias

11

I think the develop-nightly branch is supposed to turn out a nightly release, for those wanting the most advanced code. @Richardklasens_admin does this happen automatically, or does that have to be done manually?

For protection reasons, to prevent accidental pushes to master, the only way to update master is to do a PR. Push access is shut off.

12

Brian does get pull requests from time to time that don’t merge cleanly or break things. Really it is up to the pull requester to ensure he has either pulled the latest of Brian’s development branch (which is a challenge if it’s private), or rebased. In other words pull requests need to be completely clean.

I’m not the biggest fan of how github works. When you clone AgOpenGPS, and then check it out, you end up with only your github clone as a remote branch, when really you need the upstream one as well. I guess github wants you to do everything through their web interface, which is definitely not how I work.

Anyone is welcome to clone QtAgOpenGPS and work on it (checking with David or me for which branch to work off off), but you’ll have to pull from my repo before trying to get anything merged.

1 Like
13

And my opinion is that in a distributed development environment such as open source, no one should be able to push to Brian’s master, or to Brian’s repo at all. Everyone should have their own working repo locally, and their own github clone that they push to. When they want their work merged (after being rebased or merged properly with upstream), a pull request then can go to Brian’s repo. Alternatively if a developer is particularly active, Brian could add your repo as a remote and pull from his end. But that’s just my opinion.

3 Likes
14

Yes, I think this is the intention, but I don’t think that this is how it should work: we just introduced a develop branch to simplify the contribution experience and now there is also a develop-nightly… Which branch should I target? What is the difference between the two? Who will merge them back together?

For protection reasons, to prevent accidental pushes to master, the only way to update master is to do a PR. Push access is shut off.

Oh, of course this is another very valid reason to always work with PRs. Ignore this point then.

Yes, I absolutely agree. And again, for this to happen the preconditions are:

  • As a contributor I have to know beforehand, which branch to target (which we achieved by introducing the develop branch).
  • There should be no other long-lived feature branches that will eventually create conflicts when they are merged together.

I guess github wants you to do everything through their web interface, which is definitely not how I work.

I don’t think that this is true at all. When contributing to a GitHub project, the only thing that needs to happen in the web interface is creating the fork and later creating the pull request. Everything else can be done locally via terminal (and by using a tool like GitHub CLI you could even do the other things without a webbrowser).

[…] (checking with David or me for which branch to work off off) […]

I would also recommend that you do not have this prerequisite for contributing. There is no need for prior communication, before contributing to the project. Setup a develop branch and make it obvious, that this is the branch that needs to be worked off of.

And my opinion is that in a distributed development environment such as open source, no one should be able to push to Brian’s master, or to Brian’s repo at all. Everyone should have their own working repo locally, and their own github clone that they push to.

Agreed, this is also how most open-source software works.

15

Hi Matthias.

We sort of introduced the Git branching model.
Needs some changes for Brian how used to work.

Develop is the branch to base from.
Nightly is going to be deleted.

Brian works in its own branch called Arizona, if he made changes to it, it will be merged to dev when it’s stable to test.
In the mean time, dev will be behind his or others changes. I’m not sure how to solve that

16

I totally understand that it is not so easy to change how you work on the project. However, I think the benefits are worth it in the long run.

Nightly is going to be deleted.

:+1:

Brian works in its own branch called Arizona, if he made changes to it, it will be merged to dev when it’s stable to test.

Again, I would strongly advise to not have any other long-lived branches. Otherwise you will always be merging from one branch to the other, creating a lot of “noise”, instead of rebasing on develop whenever new work is started.

In the mean time, dev will be behind his or others changes. I’m not sure how to solve that

I don’t think there is a good solution to this, except to push changes to develop as soon as possible (and really treat it as the “potentially unstable” branch that it is).