Netgate SG-1000 microFirewall

Author Topic: submitting PRs for packages in the pfSense/FreeBSD-ports repo?  (Read 515 times)

0 Members and 1 Guest are viewing this topic.

Offline luckman212

  • Hero Member
  • *****
  • Posts: 726
  • Karma: +59/-0
    • View Profile
    • @luckman212 - github
submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« on: October 03, 2017, 01:51:39 pm »
I'm a relative beginner at git/GitHub and still learning new things every day. So forgive my ignorance on this!

I was trying to submit a small patch for the HAProxy package today and ran into some trouble. I worked around it but am not sure if I've done so correctly. My typical workflow when working on patches to the main pfsense/pfsense repo has been:

1) fork the main repo on GitHub
2) clone my fork of the repo
3) hack hack hack
4) commit + push

When I tried that on the pfsense/FreeBSD-ports repo, I ended up with a clone that was missing all of the files I needed. Specifically, all of the pfSense-pkg-xxxx directories which contained the files I was trying to update were just not there. What I ended up doing instead was cloning the original repo, hacking on that, and then pushing the commits, like this:

Code: [Select]
git clone git@github.com:pfsense/FreeBSD-ports.git my-foobar-patch
cd my-foobar-patch
git remote set-url origin git@github.com:luckman212/FreeBSD-ports.git
git checkout -b my-foobar-patch

~ HACK HACK HACK ~

git ls-files --modified | xargs git add
git commit
git push origin my-foobar-patch

That worked.  So, questions are:

1) why did that happen?
2) have I done this the "right" way?

Thanks all

Offline heper

  • Hero Member
  • *****
  • Posts: 2690
  • Karma: +253/-11
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #1 on: October 03, 2017, 02:27:13 pm »
I don't use git enough to remember how/why I get it to fuck up.

For small changes like this, I find it much easier to use the online editor on GitHub

Offline luckman212

  • Hero Member
  • *****
  • Posts: 726
  • Karma: +59/-0
    • View Profile
    • @luckman212 - github
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #2 on: October 03, 2017, 02:39:44 pm »
Forgot to add -- I tried that first, and GitHub gave me an error. Wish I took a screenshot of that, but TL;DR it failed.  Even if you use the online editor, it requires you to have your own fork of the project so that doesn't really get to the root of the issue.  My best guess at this point is that maybe I had an old fork of FreeBSD-ports before the "default" branch was switched to 'devel'? I do see that the 'master' branch does not contain the pfSense-pkg-xxx dirs, only devel. So maybe my fork was old and when I cloned and rebased it just sync'd to master. Who knows, I have since deleted the fork and all the branches and started w/ a clean slate.

Still looking for an answer to question #2 -- is this the "right" way to hack on pfSense?

Offline bmeeks

  • Hero Member
  • *****
  • Posts: 3148
  • Karma: +816/-0
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #3 on: October 03, 2017, 03:23:17 pm »
FIrst question ... are you trying this using the Windows Git client or are you using a Linux workstation?  I ran into the exact same problem with the Windows Git client.  It just does not like the FreeBSD-ports repository for some reason (or at least it never worked for me).  I instead use a CLI based version of Git on a FreeBSD virtual machine and everything works fine.

You should submit pull requests against the DEVEL repository for FreeBSD-ports.  The pfSense guys will do the merging to the pfSense release branch.

Bill

Offline luckman212

  • Hero Member
  • *****
  • Posts: 726
  • Karma: +59/-0
    • View Profile
    • @luckman212 - github
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #4 on: October 03, 2017, 03:25:26 pm »
Using macOS - git 2.14.2 from Homebrew

Offline bmeeks

  • Hero Member
  • *****
  • Posts: 3148
  • Karma: +816/-0
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #5 on: October 03, 2017, 03:28:19 pm »
Since Mac is an OpenBSD derivative, I would think it should work but maybe not.  If you have access to a Linux machine or a FreeBSD virtual machine, try that using the CLI version of Git.  One issue may be the huge number of directories and files in the repository.  That's what appeared to be tripping up Windows.

Bill

Offline luckman212

  • Hero Member
  • *****
  • Posts: 726
  • Karma: +59/-0
    • View Profile
    • @luckman212 - github
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #6 on: October 03, 2017, 03:33:06 pm »
Git didn't throw any errors when cloning (the error happened on GitHub's site actually). But I'll spin up a VM and do some more testing. Thanks

Offline bmeeks

  • Hero Member
  • *****
  • Posts: 3148
  • Karma: +816/-0
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #7 on: October 03, 2017, 03:36:41 pm »
I'm a relative beginner at git/GitHub and still learning new things every day. So forgive my ignorance on this!

I was trying to submit a small patch for the HAProxy package today and ran into some trouble. I worked around it but am not sure if I've done so correctly. My typical workflow when working on patches to the main pfsense/pfsense repo has been:

1) fork the main repo on GitHub
2) clone my fork of the repo
3) hack hack hack
4) commit + push

When I tried that on the pfsense/FreeBSD-ports repo, I ended up with a clone that was missing all of the files I needed. Specifically, all of the pfSense-pkg-xxxx directories which contained the files I was trying to update were just not there. What I ended up doing instead was cloning the original repo, hacking on that, and then pushing the commits, like this:

Code: [Select]
git clone git@github.com:pfsense/FreeBSD-ports.git my-foobar-patch
cd my-foobar-patch
git remote set-url origin git@github.com:luckman212/FreeBSD-ports.git
git checkout -b my-foobar-patch

~ HACK HACK HACK ~

git ls-files --modified | xargs git add
git commit
git push origin my-foobar-patch

That worked.  So, questions are:

1) why did that happen?
2) have I done this the "right" way?

Thanks all

As for question #2 --

What you have is fine with a couple of tweaks as follows.  First, I usually just use "git commit -a" instead of searching for modified files manually.  Git knows what files have been touched and the "-a"  switch will automatically include them in the commit.

The bigger tweak you need is to create a new branch each time you start a patch stream.  For example, when I am getting ready to create an update for the Snort package I do the following --

Code: [Select]
git pull upstream devel
git checkout -b pfSense-pkg-snort-3.2.9.5_2

This first pulls any updates from the main FreeBSD-ports repository into my local copy.  The next step creates a new branch in my local copy to match the package name and package version I will be submitting changes against.

When I'm ready to commit my updates I do this:

Code: [Select]
git commit -a
git push origin pfSense-pkg-snort-3.2.9.5_2

These two lines will commit all my modified files and then push them as a new branch to my Git hub fork of the FreeBSD-ports repository.  I then go to the Git Hub web site and my new branch will show up in remote repository along with a button to create a Pull Request against the upstream FreeBSD-ports repository of pfSense.

The above examples assume pfSense-pkg-snort-3.2.9.5_1 is the current devel version and pfSense-pkg-snort-3.2.9.5_2 is my updated package to be submitted as a Pull Request.

Bill

Offline luckman212

  • Hero Member
  • *****
  • Posts: 726
  • Karma: +59/-0
    • View Profile
    • @luckman212 - github
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #8 on: October 03, 2017, 03:42:33 pm »
Thanks for your tips - very helpful.  :)

Offline bmeeks

  • Hero Member
  • *****
  • Posts: 3148
  • Karma: +816/-0
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #9 on: October 03, 2017, 03:48:20 pm »
Thanks for your tips - very helpful.  :)

Another helpful tip that benefits the pfSense guys during code review is to break your package changes into a series of commits instead of one giant commit.  For instance, let's say I'm fixing four bugs in the Snort GUI code and I plan to include them all in the same update.  I will fix bug #1, then do a commit so that only the files touched while fixing bug #1 get marked.  Next I fix bug #2 and commit again.  Rinse and repeat for the other two bugs.  At the end I have a chain of four commits for bug fixes.  I will then do a final commit that includes only the file changes needed to bump the package version info (typically just one or two edited lines in the Makefile).  So the entire package update would consist of five separate commits into the same branch.

Now as folks review the changes, everything is clearly laid out and easy to follow with one commit per change.  Take a look at some of the closed pull requests on the Git Hub site for the pfSense/FreeBSD-ports repository for examples.

Bill

Offline jimp

  • Administrator
  • Hero Member
  • *****
  • Posts: 21379
  • Karma: +1432/-26
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #10 on: October 04, 2017, 08:38:43 am »
Another helpful tip that benefits the pfSense guys during code review is to break your package changes into a series of commits instead of one giant commit.

While that is nice for review, it makes it more difficult for us to cherry-pick that change to other branches, such as copying the commit(s) from devel to RELENG_2_4_0, RELENG_2_3, and RELENG_2_3_4.

It's nice to have them all squashed into a single commit before it's merged, but we have some scripts that help us deal with that so it's not the end of the world.
Need help fast? Commercial Support!

Co-Author of pfSense: The Definitive Guide. - Check the Doc Wiki for FAQs.

Do not PM for help!

Offline luckman212

  • Hero Member
  • *****
  • Posts: 726
  • Karma: +59/-0
    • View Profile
    • @luckman212 - github
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #11 on: October 04, 2017, 08:41:46 am »
Thanks jimp, so - single unified commits are preferred for PRs then?

Offline jimp

  • Administrator
  • Hero Member
  • *****
  • Posts: 21379
  • Karma: +1432/-26
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #12 on: October 04, 2017, 09:07:54 am »
Since we don't merge on github it's a little more complicated, but IIRC you can squash the commits in your PR after review so you can have multiple commits while you work, and then it can be merged as one.

https://github.com/blog/2141-squash-your-commits
http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request
Need help fast? Commercial Support!

Co-Author of pfSense: The Definitive Guide. - Check the Doc Wiki for FAQs.

Do not PM for help!

Offline doktornotor

  • Hero Member
  • *****
  • Posts: 8553
  • Karma: +956/-278
  • Not a pfSense employee, they cannot fire me...
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #13 on: October 04, 2017, 02:20:28 pm »
but IIRC you can squash the commits in your PR after review

That's what you would need to activate, none of the people submitting PRs can use that. It can only be used on merge. You have kind of an issue with the workflow you are using.
Do NOT PM for help!

Offline jimp

  • Administrator
  • Hero Member
  • *****
  • Posts: 21379
  • Karma: +1432/-26
    • View Profile
Re: submitting PRs for packages in the pfSense/FreeBSD-ports repo?
« Reply #14 on: October 04, 2017, 02:37:35 pm »
but IIRC you can squash the commits in your PR after review

That's what you would need to activate, none of the people submitting PRs can use that. It can only be used on merge. You have kind of an issue with the workflow you are using.

When I'm working locally, I can squash several commits down to one. I think the same thing can be done in a PR by the submitter, but I'm not 100% on that. I know for certain it can be done before submitting the PR. There are lots of how-tos out there on squashing, such as https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit -- Yes, github can do that automatically when doing a merge in their GUI, but that doesn't help us.

The workflow has to be the way it is because everything has to be sourced from our local gitlab instance. Aside from it being a better flow for us internally, we don't like being stopped dead in our tracks if github goes down, which happens a lot. Some people don't notice how often it happens, but when you're working in git all day, every day, it's more apparent.

There may also be something we can cook up eventually with local scripts to squash but I'm not sure any of us have looked at it.
Need help fast? Commercial Support!

Co-Author of pfSense: The Definitive Guide. - Check the Doc Wiki for FAQs.

Do not PM for help!