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

Revisit Nuttx Build #6881

Closed
davids5 opened this issue Mar 23, 2017 · 19 comments · Fixed by #7007
Closed

Revisit Nuttx Build #6881

davids5 opened this issue Mar 23, 2017 · 19 comments · Fixed by #7007
Assignees

Comments

@davids5
Copy link
Member

davids5 commented Mar 23, 2017

@dagar

Moving this here.

Revisiting the build is fine.

We still need to maintain the make oldconfig and menuconfig in whatever we do.
We need to keep in mind that a defconfig changes need a clean build.
We need to keep in mind that a patch change needs a clean copy of the affected files in the patch.

Getting the incremental build to work when switching branches would be nice if it can be done.
Getting the incremental build to work making edits in the NuttX subtree would be nice.

One gotcha is debugging via eclipse in the build dirs. If one edits the build dirs copy of the file without realizing it you can lose your edits (and your mind).

It would great if we could fix that or offer to merge the changes. The problem will be the patched files.
Still a copy back would be prefered.

@dagar
Copy link
Member

dagar commented Mar 23, 2017

What about applying the patches in place?
Does Nuttx have an out of source build?

@davids5
Copy link
Member Author

davids5 commented Mar 23, 2017

What about applying the patches in place?

We did that before cmake.
The was an objection: why is "NuttX" dirty? - Finding a git based solution as opposed to patches would be the answer. But I need the same level of control I get with the patches. Multi branches tied to different Nuttx submodels, support of multiple submodules and in multiple repos.

Does Nuttx have an out of source build?

No and since we iterate board configs as we build. in tre would be
``
distclean
cp configs in
build
export (create zip)
unzip export


Whis is exactly what we had before cmake.

@lucasdemarchi
Copy link
Contributor

@davids5 I think the way patches are applied done in #6878 is a great improvement so this can be in for now.

I'd still like to keep the patches on a branch in our fork of NuttX... this would ease how we apply newer patches, not needing the steps for "1. create patch with right prefix; 2) possibly edit the path; 3) copy do PX4/Firmware".

Since now the patches are sorted, we can easily create a branch on NuttX that has these patches applied. Then we can remove the infra to apply the patches on CMake and rather work with submodule updates.

After that I think the next step is to add support for building NuttX out of tree, so we don't need the export + unzip sequences. This may give us some trouble if upstream doesn't care about it. If it proves too troublesome we can think of a way to have in-tree build with the right set of .gitignore files so it doesn't cause the dirty submodule you mentioned.

@davids5
Copy link
Member Author

davids5 commented Mar 23, 2017

@lucasdemarchi

I am really glad you are looking this. I have a workable solution that allows me to continue to develop, with now a minimal impact on non NuttX developers.

I'd still like to keep the patches on a branch in our fork of NuttX... this would ease how we apply newer patches, not needing the steps for "1. create patch with right prefix; 2) possibly edit the path; 3) copy do PX4/Firmware".

For a fork that the maintainer updates they can do as they like.
We can even add support to the build to turn off the patches.

But this will not work for my work flow and development needs.

  1. My upstream tracking development gets broken often by upstream non autonomous commits that can persist for days or weeks. I am speaking about massive breaking API and functional changes done on master the do not have a clean commit history.

While upstream does look out for thing that my creator PX4 and give me heads up, we can not change the upstream workflow or practices.

  1. Not all patches are public. Therefore XYZ/Firmware needs to drive the changes so that a common NuttX can be used. Otherwize each XYZ team will need a PX4 nuttx fork and a maintainer.
    That may work for your team but it does not scale for all teams.

Please keep in mind - we have no upstream support in changing the build. It is recursive Make with no degree of freedom. The export build it the only upstream supported facility (and we carry a patch for that)

Also I need to have a functional git environment so if there is a conditional .gitignore - that would be great.

If you would propose a step-by-step workflow - I would be happy to see if I can use it in all the use cases I have.

My current work flow on the upstream branch of NuttX and it's submodules.

  1. Update NuttX's submodules only upstream from upstream master
    foreach subfolder git fetch and pull

  2. Review all arch/changes in supported archs I support for all vendors.

  3. run oldconfig and on all 20+ board configs.
    foreach board make target oldconfig_target

  4. Build all 20+ board configs.
    make check

  5. Mitigate changes. (this will likely generate patches to keep me working)
    git diff --no-prefix --dst-prefix=NuttX/nuttx/ --src-prefix=NuttX/nuttx/ >>../../nuttx-patches/wip_inflight_to_upstream.patch
    Submit applicable fixes to upstream - once upstreamed and settled start at 1 again.

  6. Test on 3 major targets px4fmu-v3_default px4fmu-v5_default, and others.

  7. Push all submodules.
    foreach subfolder of NuttX git push

  8. commit on NuttX to the new nuttx submodules.
    git add [nuttx] [apps] [...]
    git commit Document PX4 contribs and whatever else came in

  9. Push NuttX
    git push origin upstream

  10. On PX4/Firmware (branch) Update / commit defconfig changes and NuttX submodule.
    git add NuttX
    git commit Document PX4 contribs and whatever else came in.
    git add patches
    git commit
    git add and commit each board change

  11. Push PX4/Firmware (branch)
    git push origin branch

  12. Wait for CI -

rinse-lather-force push and repeat. :)

Out of the above I get the applicable changes and backports this to FMU/Firmware master.
git fetch origin
git checkout master
git pull origin master (I know old habits die hard)
git checkout -b master_<branchname>

gitsync (git submodule sync --recursive && git submodule update --init --recursive)
Depending on what changed work in that NuttX subfolder
git checkout -b <basegithash>_branchname
foreach change (all related hashes)
git cherry-pick changed-hashes
git diff \<basegithash\> --no-prefix --dst-prefix=NuttX/nuttx/ --src-prefix=NuttX/nuttx/ >../../nuttx-patches/< new or update> ( I suppose I should be using git format-pathch here)

Removed cruft and breaking changes from patches.
commit and push

Once CI is happy
rebase all repos I support.

@lucasdemarchi
Copy link
Contributor

Hi @davids5 .. I think what I'm suggesting is basically replacing the step 10 and 11. I prototyped it here to see if it would work and apparently it did. I'm pushing for you to take a look.

Instead of generating the patch, copying it to nuttx-patches dir and making the build system to manually apply them, this would be replaced with:

  1. Switch to branch px4 on the submodule that's being patched and backport the patch (or create the patch that's not material for upstream).
  2. In NuttX, update the submodule
  3. In PX4/Firmware, update the submodule.

With this there's no need anymore to create the patch files and instruct the build system to apply them. It's implicitly done by updating the submodule. Let me know what you think.

https://github.com/lucasdemarchi/Firmware/commits/pr-build-nuttx
https://github.com/lucasdemarchi/PX4NuttX-1/commits/px4
https://github.com/lucasdemarchi/nuttx/commits/px4
https://github.com/lucasdemarchi/apps/commits/px4

Since you already have to split the commits per submodule in order to send upstream, I don't see this as a problem, but as I said I don't want to disrupt your workflow since you are the one doing the heavy lift work.

@lucasdemarchi
Copy link
Contributor

And for example, to add the changes from @zehortigoza would be basically to apply the branch
https://github.com/lucasdemarchi/nuttx/commits/pr-zeh-changes on the px4 branch that we would maintain and propagate the submodule updates.

That branch had one conflict when I was backporting it, which is easier to solve with the git tools. And the branch is built-tested only. @zehortigoza can you take a look if I got the conflict correctly?

@davids5
Copy link
Member Author

davids5 commented Mar 24, 2017

@lucasdemarchi Can we discuss this on a call?

@davids5
Copy link
Member Author

davids5 commented Mar 24, 2017

Hi @lucasdemarchi

I was thinking about the discussion we had, and I am looking forward to seeing the workflow (with commands) laid out.

Please set me straight if I am missing the point, or some git skills/tools that invalidate the following:

But this is what I see:

A B PX4NuttX - (the knot)
 +A B NxWidgets 
 +A B apps
 +A B misc/tools
 +A B nuttx

Assuming I have only:
(A) master PX4 tied to one currently a FIXED commit on PX4NuttX
and
(B) master_some_arch_tied_to_upsteam_nuttx to my FLOATING upstream branch commit on PX4NuttX

A needs all {REJECTS,BACKPORTS, PENDING}
B needs all {REJECTS, PENDING}

I think the proposal would require me to carry, maintain and keep in sync 10 instances of branches.

A) 5 repos with one workflow - commits history is are applied patches.
B) 5 with I think??? a rebased workflow on upstream master or we lose the changes in the history.

With my current solution I have 2 to branches to maintain and (2 A&B) single directories to manage.
The operation for B is always foreach dir git pull upstream master

These 2 patch directories answers the question: How is this PX4 branch of nuttx different from upstream NuttX?

Patches:
There is no different workflow required to answer that question for A & B.
Git: A seems easy, the last commits are the deltas. B where are they in the 6-12-25 month cycle? On top if we use a rebase workflow. I guess the force pushes would be ok if I was the only one on B.

I think I can never force push on A. - correct?

Patches: On B When a patch fails can see the intent in the patch and simply redo it.
Git:I have potentially multiple merge failures (ultimately sqush-able) but instead of looking at the diff. I have to use git blame and reconcile the conflict in an N way diff - or do i have that wrong?

Also to benefit from CI on A I would need to feature branch to not clobber master after updating the PXNuttX submodules and the NuttX submodule commit.

Do I have this right?

@davids5
Copy link
Member Author

davids5 commented Apr 6, 2017

@dagar - I think the parallel build is not respecting the patch ordering. Per our chat I am noting that here.

@dagar
Copy link
Member

dagar commented Apr 13, 2017

@davids5 @lucasdemarchi I was playing around with git and it makes it really easy to go back and forth between commits and patch files.

For example, if you were working in a branch on top of upstream (I tagged it upstream) you could turn all commits into patches with a single git format-patch. I omitted some details like path setup.

git format-patch upstream

/home/dagar/git/Firmware/nuttx-patches/nuttx/0001-REJECTED-add-math.h.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0002-REJECTED-fix-shadow-wanings.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0003-REJECTED-avoid-export-copy-with-export-insitu.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0004-REJECTED-support-c-11.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0005-REJECTED-cstdint-fix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0006-REJECTED-static-assert-fix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0007-REJECTED-ctype-fix-shadow-wanings.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0008-BACKPORT-stm32-flash-F4-dcache-corruption-fix-no-HSI.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0009-BACKPORT-priority-restoration-fix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0010-BACKPORT-stack-coloration-overreach-fix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0011-BACKPORT-stm32-serial-dma-hotfix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0012-BACKPORT-i2c-hotfix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0013-BACKPORT-stm32f7-DTCM.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0014-BACKPORT-fix-CRTSCTS-defines.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0015-BACKPORT-cdcacm.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0016-BACKPORT-stm32-serial-break.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0017-BACKPORT-stm32-rcc-keep-HSI-on.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0018-BACKPORT-ramtron-CONFIG-prefix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0019-BACKPORT-stm32f3x-add-BKP.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0020-BACKPORT-stm32-bkp-reference-fix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0021-BACKPORT-stm32f7-bkp-reference-fix.patch
/home/dagar/git/Firmware/nuttx-patches/nuttx/0022-BACKPORT-stm32f7-serial-dma-hotfix.patch
/home/dagar/git/Firmware/nuttx-patches/apps/0001-REJECTED-add-var-expansion-in-nsh-parse.patch
/home/dagar/git/Firmware/nuttx-patches/apps/0002-REJECTED-silence-jobserver-warnings.patch
/home/dagar/git/Firmware/nuttx-patches/apps/0003-BACKPORT-add-set-ex-to-nsh.patch

You can also apply all patches as a commit each with one command.

git am *.patch

Applying: REJECTED-add-math.h
Applying: REJECTED-fix-shadow-wanings
Applying: REJECTED-avoid-export-copy-with-export-insitu
Applying: REJECTED-support-c++11
Applying: REJECTED-cstdint-fix
Applying: REJECTED-static-assert-fix
Applying: REJECTED-ctype-fix-shadow-wanings
Applying: BACKPORT-stm32-flash-F4-dcache-corruption-fix-no-HSI-on
Applying: BACKPORT-priority-restoration-fix
Applying: BACKPORT-stack-coloration-overreach-fix
Applying: BACKPORT-stm32-serial-dma-hotfix
Applying: BACKPORT-i2c-hotfix
Applying: BACKPORT-stm32f7-DTCM
Applying: BACKPORT-fix-CRTSCTS-defines
Applying: BACKPORT-cdcacm
Applying: BACKPORT-stm32-serial-break
Applying: BACKPORT-stm32-rcc-keep-HSI-on
Applying: BACKPORT-ramtron-CONFIG-prefix
Applying: BACKPORT-stm32f3x-add-BKP
Applying: BACKPORT-stm32-bkp-reference-fix
Applying: BACKPORT-stm32f7-bkp-reference-fix
Applying: BACKPORT-stm32f7-serial-dma-hotfix
Applying: REJECTED-add-var-expansion-in-nsh-parse
Applying: REJECTED-silence-jobserver-warnings
Applying: BACKPORT-add-set-ex-to-nsh

With corresponding git commits.

* 4ce6edd - (HEAD) BACKPORT-stm32f7-serial-dma-hotfix (11 minutes ago) <Daniel Agar>
* 45952c1 - BACKPORT-stm32f7-bkp-reference-fix (11 minutes ago) <Daniel Agar>
* 4ad4d0b - BACKPORT-stm32-bkp-reference-fix (11 minutes ago) <Daniel Agar>
* 8d7fddd - BACKPORT-stm32f3x-add-BKP (11 minutes ago) <Daniel Agar>
* c75697d - BACKPORT-ramtron-CONFIG-prefix (11 minutes ago) <Daniel Agar>
* c74a31c - BACKPORT-stm32-rcc-keep-HSI-on (11 minutes ago) <Daniel Agar>
* 00ae5e5 - BACKPORT-stm32-serial-break (11 minutes ago) <Daniel Agar>
* 7d58183 - BACKPORT-cdcacm (11 minutes ago) <Daniel Agar>
* 007a8c2 - BACKPORT-fix-CRTSCTS-defines (11 minutes ago) <Daniel Agar>
* 63fe8fb - BACKPORT-stm32f7-DTCM (11 minutes ago) <Daniel Agar>
* 13ed09d - BACKPORT-i2c-hotfix (11 minutes ago) <Daniel Agar>
* 60952a0 - BACKPORT-stm32-serial-dma-hotfix (11 minutes ago) <Daniel Agar>
* 3161058 - BACKPORT-stack-coloration-overreach-fix (11 minutes ago) <Daniel Agar>
* 28a7865 - BACKPORT-priority-restoration-fix (11 minutes ago) <Daniel Agar>
* 731a73f - BACKPORT-stm32-flash-F4-dcache-corruption-fix-no-HSI-on (11 minutes ago) <Daniel Agar>
* 2e2a354 - REJECTED-ctype-fix-shadow-wanings (11 minutes ago) <Daniel Agar>
* 9932d93 - REJECTED-static-assert-fix (11 minutes ago) <Daniel Agar>
* 44e388d - REJECTED-cstdint-fix (11 minutes ago) <Daniel Agar>
* 0b43885 - REJECTED-support-c++11 (11 minutes ago) <Daniel Agar>
* a5c73c7 - REJECTED-avoid-export-copy-with-export-insitu (11 minutes ago) <Daniel Agar>
* be8b038 - REJECTED-fix-shadow-wanings (11 minutes ago) <Daniel Agar>
* 4f04b44 - REJECTED-add-math.h (11 minutes ago) <Daniel Agar>
*   8b81cf5 - (tag: upstream) Merged in david_s5/nuttx-3/david_s5/typo-in-stm32f76xx77xx_pinmaph-edited-on-1481298811328 (pull request #182) (4 months ago) <Gregory Nutt>

Each patch file is the same diff, but would also include the committer, date, and commit message.
https://github.com/dagar/Firmware/blob/nuttx_test/nuttx-patches/nuttx/0001-REJECTED-add-math.h.patch

So we could really get the best of both worlds with this approach. @davids5 could shuffle patches to keep track of these things, but also trivially apply them to a branch off of PX4's NuttX repo. Additionally, if we don't have to handle patches for every single NuttX build I have a solution that will allow you to edit NuttX files in place.

https://github.com/dagar/Firmware/tree/nuttx_test

@lucasdemarchi
Copy link
Contributor

@dagar yep. It much easier to keep the commits on a git branch. This is basically what I did on the branches pointed above. As I said the difference is basically if patches are kept as separate files or in a branch. Keeping them on a branch means we don't have to apply them at every build and avoid all headaches that this entails (see the amount of issues that this causes).

The only downside is that Nuttx itself has submodules and thus you can't have patches crossing submodules. However since the end goal is to upstream the patches, they should already be split on submodule boundary anyway

@davids5
Copy link
Member Author

davids5 commented Apr 13, 2017

@dagar , @lucasdemarchi

I was thinking about the discussion we had, and I am looking forward to seeing the workflow (with commands) laid out.

Please see the #6881 (comment) If you both a have a` complete solution, that supports the work I have described above. Please answer the questions and provide a complete workflow with all the commands. I need to see the end-to-end solution. The pieces are nice. But without a complete picture. I can not tell if you have complete solution that is more or less work with the same number of branches and level of support.

In your scenarios: Assume the current nuttx (44ad7e224c1ef17911ab8b4101fd624ad9ee4177)
has only back port development only. It is Nuttx + rejects + backports.

The upstream branch of NuttX - is always the latest upstream. That has say PX4 master_nxphlite tied to it.

When dev is done on NuttX upstream it is branched as upstream_kinetis
upstream_kinetis is the WIP NuttX branch that is rebased on upstream NuttX and has the work in progress for that processor. That will go upstream and get picked up in the next upstream uptake.

While that PR (opr patch set) is inflight to upstream, master_nxphlite is tied back to NuttX upstream and the patches are placed in a pending (wip_inflight) patch file. Once that PR is merge into master upstream. wip_inflight is deleted and master_nxphlite is then tied and built against upstream. This can happen 5 time a day.

Only the rejects and pending patches are in PX4/Firmware master_nxphlite .
master_nxphlite bounces between upstream_kinetis and upstream and the pending patches come and go.

master_nxphlite is constantly rebased on PX4 master.

Using 3 px4 repos say PX4, NXP, INTEL with only PX4 having a NuttX submodules. NXP and INTEL just reference PX4/NuttX. INTEL and NXP repos may have different patches, depending on the development phase.

Please describe in detail the work flow in maintaining PX4, NXP, INTEL. for current NuttX and upstream

@lucasdemarchi
Copy link
Contributor

Basically what I'm suggesting is "Don't change your workflow, just change how patches are applied in the end".

This means that you only replace the last step on your workflow: instead of copying a .patch file to PX4/Firmware/nuttx-patches you just switch to the px4 branch on that repository and commit. Example:

a) you added a bug fix
b) you generate a patch
c) you send it to upstream
d) you copy it to PX4/Firmware
e) you commit there and raise a PR

You would change (d) and (e) by:

d) switch to px4 branch
e) commit there and raise a PR updating the submodule

Notice that on (d) you will already be on top of your pile of changes before, so you can check for conflicts with previous changes at time of applying the patch rather than having to build it and hope things apply cleanly.

With this little change it's possible to remove all the infrastructure from the build system to apply patches
(for everyone) while building. This is exactly what I've done on the branches I pointed out:
https://github.com/lucasdemarchi/Firmware/commits/pr-build-nuttx
https://github.com/lucasdemarchi/PX4NuttX-1/commits/px4
https://github.com/lucasdemarchi/nuttx/commits/px4
https://github.com/lucasdemarchi/apps/commits/px4

(these branches are outdated, but I can easily update them if we want to at least try this approach).

All the issues with patch order, handling dependencies, etc, go away.

Look how this has nothing to do with having repos PX4, NXP, INTEL... this is a non-issue: there are only 2 interesting repositories: upstream NuttX and px4 branch.

dagar added a commit to dagar/PX4-Firmware that referenced this issue Apr 17, 2017
dagar added a commit that referenced this issue Apr 18, 2017
@lucasdemarchi
Copy link
Contributor

@dagar why did you close this? This issue is about changing the workflow, not about was committed on #7007. I'm waiting on feedback from @davids5

@dagar
Copy link
Member

dagar commented Apr 18, 2017

This was referenced (and closed automatically) by #7007 because the original reason this was opened was my suggestion to replace our custom NuttX build wrapper with cmake ExternalProject_Add. I decided to just fix the dependencies in our custom wrapper instead.

Reopening.

@dagar dagar reopened this Apr 18, 2017
@davids5
Copy link
Member Author

davids5 commented Apr 18, 2017

@lucasdemarchi

Notice that on (d) you will already be on top of your pile of changes before, so you can check for conflicts with previous changes at time of applying the patch rather than having to build it and hope things apply cleanly.

We have the ordering issue resolved now.

I think what you are proposing is only applicable to the non development version (current) of NuttX. Right?

It does not make the workflow I have for "upstream" work the same. This means I still have to carry a set of commits or patches to support rejected and wip. In the case of commits - I would have to constantly rebase the affected NuttX submodules on upstream in multiple repos. With patches I can avoid that.

I am not seeing the advantage to having 2 ways to do the same thing.

@lucasdemarchi
Copy link
Contributor

I think what you are proposing is only applicable to the non development version (current) of NuttX. Right?

Not really. It applicable to easily manage multiple versions/branches of NuttX.

It does not make the workflow I have for "upstream" work the same. This means I still have to carry a set of commits or patches to support rejected and wip. In the case of commits - I would have to constantly rebase the affected NuttX submodules on upstream in multiple repos.

You don't really have to rebase. Merge and/or revert still work. Note that we are not changing the NuttX revision for very long times, so I don't see where this "constantly rebase" is coming from. Looking at the log, the last time we synced with upstream was in 07923a8 (Upgrade to Nuttx 7.18+ ==upstream)

@davids5
Copy link
Member Author

davids5 commented Apr 18, 2017

@davids5
Copy link
Member Author

davids5 commented Sep 29, 2017

Done in 1.7 NuttX upgrade

@davids5 davids5 closed this as completed Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants