Nodemcu-firmware: When to drop the next master release

[HHHartmann]

Sorry for opening an issue for this, but it seems right to discuss this in a visible place.

In https://github.com/nodemcu/nodemcu-firmware/pull/3060#issuecomment-610281199
@marcelstoer wrote:

I think it would be very valuable (and thus welcome by the community) if we merged dev back to master in the coming weeks. It's been almost six months but there are a few things pending: https://github.com/nodemcu/nodemcu-firmware/milestone/14. So, I wouldn't mind landing this on dev soonish as it looks quite mature.

I would like to see some bugs fixed before we merge into master.
Uploading LFS.img with ESPlorer does not work anymore #2963
adc.readvdd33() #2925
enduser_setup "Handling POST" failure on 310faf7 and dev #2931

Especially the upload bug will leave a good part of the community without a working master.
The readvdd33 bug is a regression and I think should be fixed rather sooner than later.
They are both fixed in Terry's branch.
Also the broken enduser_setup should be addressed.

[marcelstoer]

I would like to see some bugs fixed before we merge into master.

Definitely! That's also the reason I maintain these milestones and the reference from PR to milestone (tracking yet-to-be-merged stuff).

[nwf]

I would like #3064 to land as well before the next cut.

[marcelstoer]

It's in the milestone, yes.

[nwf]

In the interest of cutting a new release, I have dropped #2854 and #3075 from the milestone. #2854 needs work, still, and #3075 should not be part of this cut to master, but rather the next one, since it touches so many modules (with a goal of NFC, to be clear, but just in case) and has not been given a thorough shakedown.

That means, I think, that we are blocking on @HHHartmann to decide that #3069 and #2985 should land or not. Given @TerryE's resurrection of the Lua 5.3 branch, I'd like to make the drop to master approximately now so that we can then land his work on dev and have all of us test it out and do point fixes as needed.

[HHHartmann]

As far as I am concerned #3069 can go in as it is tested and imposes no risk at all.

2985 is tested only half. But with some luck it will be tested soon.

But as stated above there is at least one bug (#2963) in the current dev branch which is a showstopper and which is fixed in #3075.

It also seems wired to wait for this PR for 4 Months and then do the cut without it just the moment work on it continues.

[marcelstoer]

But as stated above there is at least one bug (#2963) in the current dev branch which is a showstopper and which is fixed in #3075.

I had suggested elsewhere to unfold it from Terry's large change set and to commit/PR it separately.

[TerryE]

@marcelstoer, it took me a solid day's work to do the current dev -> lua53 merge. It am really loath to start cherry picking little bits back out, but If someone else is willing then ... TBH I've done so much work on this branch now that I can't remember exactly what this particular fix was. :smile:

Maybe this is a good argument for leaving master as-is. I also think that we should clone master to pre-lua53 master just in case.

[marcelstoer]

TBH I've done so much work on this branch now that I can't remember exactly what this particular fix was.

I was gonna comment on that only to find out your redacted comment already did 😉 I checked the commits in both #3075 and #2912 and came to the same conclusion. If at all only you would find that in reasonable time I guess. And then even if someone goes through all the comments in #2963 to figure out what you changed and manually fixes it for dev it would probably cause merge conflicts in your branch.

[TerryE]

I've just checked and ESPlorer binary upload works fine on the new PR branch for both the Lua51 and Lua53 variants.

[TerryE]

Maybe @nwf Nathaniel can form a view on whether he is comfortable with the new PR at least in the Lua51 variant, though TBH I haven't had any probs yet with the 5.3 build either. In the meantime I will press on with the tidying up.

[nwf]

@HHHartmann

As far as I am concerned #3069 can go in as it is tested and imposes no risk at all.

3069 is tested only half. But with some luck it will be tested soon.

Did you mean to give the same two numbers? Which one should have been #2985? How soon is "soon" anyway? If we are in a rush to cut master, ideally the answer is something like "by noon GMT Sunday".

As to master-cutting #3075... I am absolutely happy to have it come to dev now (or actually yesterday, even), and I will not attempt to veto a consensus that cutting master should happen thereafter rather than before, as seems to be desired.

[HHHartmann]

@HHHartmann

As far as I am concerned #3069 can go in as it is tested and imposes no risk at all.

3069 is tested only half. But with some luck it will be tested soon.

Did you mean to give the same two numbers? Which one should have been #2985?

Sorry I meant #2985 as you guessed already. Also fixed this in the original post.

How soon is "soon" anyway? If we are in a rush to cut master, ideally the answer is something like "by noon GMT Sunday".

I can't tell as another contributor said he would test it. Anyway, no need to wait for this PR.

[marcelstoer]

All the pending issues have been fixed. Meaning: all of the PRs "planned" for the next drop landed on dev. According to our own guidelines we should now keep our hands off dev for another two weeks to see if we incidentally broke anything.

[TerryE]

@marcelstoer, one of the things that I need to do for the dev -> master PR is to cross check the two at a files level to make sure that we haven't dropped or are missing any updates that should be going in -- as possibly happened with that telnet update.

[nwf]

It feels like things are stuck again. What can I do to help move the process along?

[TerryE]

@nwf, let me have a look tomorrow at how I can quickly tidy up and get us to a cut point.

[TerryE]

Issues that have been fixed / addressed

• #2854 net_info module - ping function. Merged and closed
• #2925 adc.readvdd33(). Fixed in #3075. Closed
• #2963 Uploading LFS.img with ESPlorer does not work anymore. Closed and fixed.
• #2985 Fix binary and chunked HTTP downloads. Add diffs from @anod221. Merged and closed
• #3069 HTML for Apple devices to mitigate POST fragmentation. Merged and closed
• #3064 More, but more subdued, rampaging for next release. Merged and closed
• #3106 cannot compile dev build with #define PMSLEEP_ENABLE. Fixed in 50fcd72. Needs to be in next release.

Issues still open

• #2931 enduser_setup "Handling POST" failure on 310faf7 and dev. Already covered by #2847 but limited to issue not fixed by #2852. **@HHHartmann to update status.
• #3118 Dev Branch Telnet, FTP Server Compatibility. I have reworked the versions in my current working branch so that the extra inline --[[SPLIT]] comments support #3117. I have just checked these in an LFS image against a current Cloud builder dev firmware image and these version will close this issue. I suggest that I use these versions TO replace the current telnet and ftpserver versions. The new fast module itself is best deferred until after the mex master drop as we still have some tweaks to do.
• #3096 SPIFFs File Corruption. I haven't been able to reproduce this yet, but I have made some changes to the main.c to align it to the current fimrware version. I feel that we should PR this before the master drop.

Issues definitely deferred after next drop

• #3123 Remove sqlite3 module

[HHHartmann]

Issues still open

• #2931 enduser_setup "Handling POST" failure on 310faf7 and dev. Already covered by #2847 but limited to issue not fixed by #2852. **@HHHartmann to update status.

We introduced a workaround here.
A final fix is not planned for this release
So no stopper for the release.

[nwf]

With #3118 now closed, I believe #3096 is now the sole blocker for a master drop. Yes?

[TerryE]

With #3118 now closed, I believe #3096 is now the sole blocker for a master drop. Yes?

IMO, #3096 is going to take lot of work to validate if there is a genuine failure mode and if so to isolate the bug. We also only have one user who has come across this, so even if it is a genuine issue, then it doesn't seem to be impacting a lot of developers.

What also concerns me here is that our file and platform/vfs.h layers are simply a wrapper around a separately maintained SPIFFS package and hunting down subtle bugs in this is going to be hard.

So my recommendation is that I raise a PR for the tools/spiffsimg/main.c alignment point now but we defer any further investigation until after the drop.

[nwf]

On the topic of the next master drop, I'd like us to do an unusual thing: after merging and tagging master, please merge master back into dev so that git describe picks up the new tag when building on the dev branch.

[marcelstoer]

@nwf I had this on my list yes. However, I feel we might be in for a few surprises (i.e. conflicts). Even though Git v2.27+ should fix the git describe shortcomings we should probably turn the merge-back into a habit rather than an exception.

[nwf]

We can do a git merge --strategy=ours back into dev to avoid any actual work happening, just creating the merge commit that points at the new master and the previous dev.

[TerryE]

I think that all of this work around SPIFFS, alternatives, extra file systems, etc. needs a co-ordinated chunk of effert. It's clear that there are a lot of good ideas could form the basis of a good improvement to the NodeMCU feature set. However, nothing here jumps out as a straightfowrard limited scope quick fix that we could credibly slip in before the master drop.

This being the case, I feel that we have really committed all that we are going to before the drop, so can I leave executing this to you @nwf and @marcelstoer etc. Thanks.

[TerryE]

@nwf @marcelstoer I've never done stuff like git merge --strategy=ours so I will leave this to you guys. Once done, I will pull the latest dev and raise the PR to cover my working branch for #3087 rebaselined against dev.