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

[FRR]: Use stg instead of patch #3480

Merged
merged 23 commits into from
Sep 29, 2019

Conversation

pavel-shirshov
Copy link
Contributor

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@lguohan
Copy link
Collaborator

lguohan commented Sep 24, 2019

[ FAIL LOG START ] [ target/debs/stretch/frr_7.0.1-sonic-0_amd64.deb ]
make[1]: Entering directory '/sonic/src/sonic-frr'
# Build the package
pushd ./frr
git reset --hard
git clean -xfdf
stg init
git checkout -b work frr/7.0
stg import -s ../patch/series
tools/tarsource.sh -V -e '-sonic'
dpkg-buildpackage -rfakeroot -b -us -uc -Ppkg.frr.nortrlib -j8
popd
mv frr-pythontools_7.0.1-sonic-0_all.deb frr-dbgsym_7.0.1-sonic-0_amd64.deb frr-snmp_7.0.1-sonic-0_amd64.deb frr-snmp-dbgsym_7.0.1-sonic-0_amd64.deb frr_7.0.1-sonic-0_amd64.deb /sonic/target/debs/stretch/
/sonic/src/sonic-frr/frr /sonic/src/sonic-frr
HEAD is now at cd305c097 Merge pull request #4334 from mjstapp/fix_vrf_lsps_7_0
stg init: Not on any branch

why this message above?

@pavel-shirshov
Copy link
Contributor Author

@lguohan
I have no idea. I spent two days looking into the issue in this PR #3017
All commits worked on my build machine, but not in the test machine. So I prefer using patch.
I tried to find the issue inside of stgit, but as I told stgit always worked on my machine, so I can't reproduce the issue.

@pavel-shirshov
Copy link
Contributor Author

This Makefile works for my build machine

.ONESHELL:
SHELL = /bin/bash
.SHELLFLAGS += -e

MAIN_TARGET = $(FRR)
DERIVED_TARGET = $(FRR_PYTHONTOOLS) $(FRR_DBG) $(FRR_SNMP) $(FRR_SNMP_DBG)

$(addprefix $(DEST)/, $(MAIN_TARGET)): $(DEST)/% :
        # Build the package
        pushd ./frr
        git reset --hard
        git clean -xfdf
        git checkout -b work origin/frr/7.0
        stg init
        stg import -s ../patch/series
        tools/tarsource.sh -V -e '-sonic'
        dpkg-buildpackage -rfakeroot -b -us -uc -Ppkg.frr.nortrlib -j$(SONIC_CONFIG_MAKE_JOBS)
        popd
        mv $(DERIVED_TARGET) $* $(DEST)/

@pavel-shirshov
Copy link
Contributor Author

The latest change will not help for #3407
I'll add the fix for it later to this PR

@lguohan
Copy link
Collaborator

lguohan commented Sep 27, 2019

looks like you fixed this issue. is the pr ready to merge?

@pavel-shirshov
Copy link
Contributor Author

@lguohan I've fixed stg issue, but don't 'second rebuild' issue. I've added new patch. Let's wait. But the new patch works for everything on my system

@@ -4,17 +4,18 @@ SHELL = /bin/bash

MAIN_TARGET = $(FRR)
DERIVED_TARGET = $(FRR_PYTHONTOOLS) $(FRR_DBG) $(FRR_SNMP) $(FRR_SNMP_DBG)
BRANCH = $(shell date +%Y%m%d\.%H%M%S)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will checkout a different branch each time FRR is built. Why is this necessary? The stg undo and git clean -xfdf should be enough to reset the repo, is it not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise I'll get
stg init: work: branch already initialized

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you not just delete the branch during the cleanup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't help.
stg creates work.stgit branch and something else.

Copy link
Collaborator

@nikos-github nikos-github left a comment

Choose a reason for hiding this comment

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

@pavel-shirshov @lguohan Will this fix work with private changes? Will they be preserved? Can you explain how it will work if someone has modified frr code and wants to re-build with the patches as well as their changes?

@pavel-shirshov pavel-shirshov marked this pull request as ready for review September 29, 2019 17:55
@pavel-shirshov
Copy link
Contributor Author

@nikos-github When I need introduce some changes I'm

  1. Make the changes in my source-tree
  2. Generate a patch with my changes
  3. Put the patch into the patchset.

@pavel-shirshov pavel-shirshov merged commit fb666d2 into sonic-net:master Sep 29, 2019
@pavel-shirshov pavel-shirshov deleted the pavelsh/stg branch September 29, 2019 17:57
@nikos-github
Copy link
Collaborator

nikos-github commented Sep 29, 2019

@pavel-shirshov @lguohan This approach is very limiting and I don't believe it addresses the issue in a generic way. If a developer has changes in frr, they still won't be able to compile. You seem to be suggesting that people should be generating a patch every time they change something, put it into the patchset, change the series, undo or stage their changes so that the checkout can work and then compile? Why are we preferring such an inflexible workflow in comparison to #3483 solution? (cc @ben-gale @sudhanshukumar22 @gpaussabrcm)

@pavel-shirshov
Copy link
Contributor Author

@nikos-github You always can develop in another branch and generate patches for the build system.

mssonicbld added a commit that referenced this pull request Aug 8, 2024
…atically (#19853)

#### Why I did it
src/sonic-utilities
```
* 7783abd1 - (HEAD -> 202405, origin/202405) Merge changes introduced in PR#3447 into branch 202405 (#3480) (4 hours ago) [Changrong Wu]
```
#### How I did it
#### How to verify it
#### Description for the changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants