Skip to content
This repository has been archived by the owner on Sep 20, 2022. It is now read-only.

add RIOT as submodule #50

Merged
merged 2 commits into from
Jan 3, 2019
Merged

add RIOT as submodule #50

merged 2 commits into from
Jan 3, 2019

Conversation

smlng
Copy link
Member

@smlng smlng commented Oct 4, 2018

This adds RIOT as a submodule fixed on the latest official release, i.e., 2018.07 2018.10. It also adapts the Makefiles of the 2 3 existing apps to use the submodule as RIOTBASE.

The idea would be that the submodule is bumped to the next version (RIOT release) as part of the release process and with that make sure all apps compile and run with the new release. This would
help to allow problems like #47 where the app is broken because of changes to RIOT master.

@smlng smlng mentioned this pull request Oct 4, 2018
@kaspar030 kaspar030 mentioned this pull request Oct 5, 2018
Copy link
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

ACK on my side

@smlng
Copy link
Member Author

smlng commented Oct 5, 2018

thx @jia200x, I think this should get more than one approval, though, or at least some discussion ...

@jia200x
Copy link
Member

jia200x commented Oct 5, 2018

thx @jia200x, I think this should get more than one approval, though, or at least some discussion ...

I agree (the ACK was intended though). We can request a small change to avoid merging by mistake.

@@ -0,0 +1,3 @@
[submodule "RIOT"]
Copy link
Member

Choose a reason for hiding this comment

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

Just add this to prevent merging before discussion. I remove it later ;)

@PeterKietzmann
Copy link
Member

Having submodules for this repo makes much sense. Would it be possible to add singe submodules for each application?

The idea would be that the submodule is bumped to the next version (RIOT release) as part of the release process

People are planning to add further applications here, e.g., a board specific application for the SenseBox and who knows where we end up when one action leads to another... What I'm trying to say is that maybe we don't want to add this to the release procedure in the long-term.

@smlng
Copy link
Member Author

smlng commented Oct 8, 2018

@PeterKietzmann

First: having submodules per application is possible, but would also mean the actual application is in a different repository. That could be a way for third party apps that we want to expose (and test) here.
This needs more thinking and discussion, how to proceed.

Second: if wee agree to add RIOT as a submodule in order to make this repo more stable, we also need to implement a process of updating this submodule in a regular fashion, which includes testing and verifying all apps compile and work. To it makes sense to make this part of the release process, which doesn't mean the release manager needs to do that but as with all the release testing may delegate this task to others.

@smlng
Copy link
Member Author

smlng commented Nov 14, 2018

so what about this one?

@jnohlgard
Copy link
Member

I think it is a good idea to use a submodule to get some improved stability here. In my opinion, the process here should be tracking the release process in the way that the RIOT release is not affected if anything stops working in this repo, but it should be a goal to update the submodule in this repo as soon as possible after a new OS release has been made upstream, including API changes to applications, so that the submodule is always pointing to the latest stable release.

In the case where there are bigger API changes affecting these applications we could have a development branch which could be using commits from the upstream master branch for the submodule, only to make the required changes to applications easier to review. The applications dev branch would then be PR:ed into the applications master when the next stable OS release is out, to handle the upstream API changes in one single swoop.

Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

coap-chat/Makefile needs an update too

@smlng
Copy link
Member Author

smlng commented Nov 14, 2018

@gebart fixed that

@smlng
Copy link
Member Author

smlng commented Dec 19, 2018

ping! It seems everybody kind of agrees that this is a good idea, right? So why not do this, then. I'll update to 2018.10 for now ...

The RIOT submodule is intended to be fixed to lastest release, in this
initial state that is 2018.10. The idea is to make sure all applications
compile and run with that release and the submodule is later on updated
by the RIOT release manager as part of the release process.
@smlng
Copy link
Member Author

smlng commented Dec 19, 2018

please note that this is also required to activate Travis-CI later, #48

Copy link
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

Looks fine to me
Tested building the apps for frdm-kw41z and mulle, I do not have the resources to run a full buildtest for each app.

@smlng
Copy link
Member Author

smlng commented Dec 21, 2018

@jia200x mind to approve or dismiss your (not-needed-anymore) blocking "review" 😄

@jia200x
Copy link
Member

jia200x commented Jan 2, 2019

maybe a third ACK to be consistent with the RIOT repository rules? :)

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030 kaspar030 merged commit 20e041d into RIOT-OS:master Jan 3, 2019
@jia200x
Copy link
Member

jia200x commented Jan 3, 2019

🎉

@miri64
Copy link
Member

miri64 commented Jan 3, 2019

I added a note to the release management draft, so we (hopefully) don't forget about this repository during the release cycle ;-).

@smlng smlng deleted the pr/submodule branch January 7, 2019 09:41
chrysn pushed a commit to chrysn-pull-requests/RIOT that referenced this pull request Sep 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants