search

esp8266 / Arduino

ESP8266 core for Arduino
GNU Lesser General Public License v2.1
15.97k stars 13.34k forks source link

Breaking change made without major version bump #5572

Closed per1234 closed 5 years ago

per1234 commented 5 years ago

Platform

Settings in IDE

Problem Description

boards.txt menu ID names were changed in https://github.com/esp8266/Arduino/commit/cc0bfa04d401810ed3f5d7d01be6e88b9011997f:

Changed menu option IDs:

This breaks previously valid fqbns (e.g. esp8266:esp8266:d1_mini:CpuFrequency=160,VTable=heap,FlashSize=4M2M,LwIPVariant=v2mss1460,Debug=Serial,DebugLevel=SSL,FlashErase=sdk,UploadSpeed=512000).

This is a breaking change, which should result in a major version bump.

I would also expect such changes to be thoroughly and prominently documented at the top of the release notes and in the FAQ.

I understand the reason for this change, but I think the documentation of breaking changes to this project needs improvement.

MCVE Sketch

NA

Debug Messages

NA

d-a-v commented 5 years ago

As you are mentionning it, it is a workaround to a windows limitation with the arduino builder which is now fixed by https://github.com/arduino/arduino-builder/commit/ab41198a9a3a9da3e04ca493fd245d0e87b4785c

This fqbn has already been changed between 2.3.0 and 2.4.x (x=0,1,2) without such request for a major version update. Could you please tell why, where or in which build system this is a breaking change ?

We also can revert it since the upstream issue is solved and the previous fqbn was more clear, if we can ask windows users to use a recent release of the arduino environment.

per1234 commented 5 years ago

This fqbn has already been changed between 2.3.0 and 2.4.x (x=0,1,2) without such request for a major version update.

If so, that's exactly my point. But were those ID changes or just the addition of new menu options? If the latter, that would probably be considered an non-breaking API change. The reason is the Arduino IDE/arduino-cli should use the default menu option when none is specified by the FQBN (e.g. https://github.com/arduino/arduino-cli/issues/23).

Could you please tell why, where or in which build system this is a breaking change ?

Arduino IDE CLI:

arduino_debug --verify --board esp8266:esp8266:d1_mini:CpuFrequency=160,VTable=heap,FlashSize=4M2M,LwIPVariant=v2mss1460,Debug=Serial,DebugLevel=SSL,FlashErase=sdk,UploadSpeed=512000 --verbose examples/01.Basics/Blink/Blink.ino
Loading configuration...
Initializing packages...
Preparing boards...
Error: CpuFrequency: Invalid option for board "d1_mini"

arduino-cli

arduino-cli-0.3.3-alpha.preview-windows compile --fqbn esp8266:esp8266:d1_mini:CpuFrequency=160,VTable=heap,FlashSize=4M2M,LwIPVariant=v2mss1460,Debug=Serial,DebugLevel=SSL,FlashErase=sdk,UploadSpeed=512000 --verbose examples/01.Basics/Blink/Blink.ino
Error: Error resolving FQBN: getting build properties for board esp8266:esp8266:d1_mini: invalid option 'CpuFrequency'
Compilation failed.

Some real life examples of this causing breakage:

We also can revert it since the upstream issue is solved and the previous fqbn was more clear, if we can ask windows users to use a recent release of the arduino environment.

It's a tough choice between backwards compatibility and API breakage. I respect whichever decision the ESP8266 team chose. Of course, breakage should always be avoided when possible, but sometimes it just needs to happen to allow the project to progress. I'm most concerned with how breakage is documented. It does sound like the implications of https://github.com/esp8266/Arduino/commit/cc0bfa04d401810ed3f5d7d01be6e88b9011997f were not fully considered at the time, so perhaps it's worth reconsidering now with all the information on hand. If it is to be reverted, the sooner the better, since going back to the old IDs is going to cause breakage for anyone who has updated.

devyte commented 5 years ago

@per1234 First, a clarification about this breaking change. Consider our release model. This was a breaking change that fell under The One Exception (Minor releases): Users were faced with not being able to build. Although this is strictly speaking not semver-compliant, in our specific case we need to allow this exception to exist Because Reasons.

However, you are correct that there should have been an entry in the Changelog for the release, and that was overlooked. To be fair, it was a time of transition for us current maintainers (still is, in fact), and at the time, the release instructions weren't formalized yet. Personally, I wasn't even aware that a change in FQBN is breaking. To avoid the oversight in the future, I will update the release instructions to include specifics for handling breaking changes. That should result in more explicit descriptions in the Changelog.

About the current state, we've discussed internally, and the decision is to keep status quo. If we were to revert, then any users who followed the change and have already adapted would be faced with a 2nd breaking change, and would need to revert as well.

per1234 commented 5 years ago

This was a breaking change that fell under The One Exception

That exception is stupid. Why are you so afraid of incrementing the major version? Version numbers should convey information but with this exception, it doesn't. I think this is pointless and disrespectful to your users. Wouldn't it be so much more simple to increment the major version on every release that contains breaking changes?

devyte commented 5 years ago

@per1234 said:

That exception is stupid

You're entitled to your opinion. We as maintainers of this repo are entitled to make policy decisions on how to move forward in accordance with development needs.

I will remind you at this point that this is not a repo where developers provide a service to users. This is a community effort, where every user is free to develop and contribute the results. Our role as maintainers is to provide direction and regulation of those contributions. In addition to maintainers we're also users, and as users we lead the contribution efforts. Both as maintainers and contributors, we volunteer our free time to do what has to be done. We don't work for Espressif, and we certainly don't get paid for what our efforts or time spent.

What that means is that we don't owe you an explanation. We discuss, we make certain decisions, and we move forward. If anyone wants to be privy to that process, that person can commit like we have, and take on a share of the workload.

Having said that, and given that the repo shows that you have in fact contributed five PRs in 3 years, I will tell you this: there is a plan in place, which includes that exception. The alternative to that plan was to freeze the repo for a very extended period of time while fundamentals got fixed. A freeze would have meant several years without releases, and not accepting new issues during that time. Personally, I consider that to be far worse to users than our current path. It certainly was pretty bad between 2.3.0 and 2.4.0.

Now, how about we drop the whole breakage subject and focus on fixing stuff? Your concern about documenting breaking changes is addressed in #5581 , although that PR is meant for use by maintainers in future releases, and not users.

ianfixes commented 3 years ago

I will echo the frustration here. By what I see in this thread, I maintain one of many libraries also affected by the breaking change -- all of which appear to be volunteer efforts in and of themselves, and all of which take unscheduled time to accommodate that breakage.

As written in the release model regarding major vs minor changes:

The one exception is breaking changes for a feature that is too broken and is not worth fixing, especially if that feature is causing maintenance overhead.

But as you pointed out in this thread:

this is strictly speaking not semver-compliant

The entire purpose of SemVer is the compliance; it allows infinite (technically, UINT_MAX) bugfixes & breaking changes to be released upstream without ever breaking downstream. So, by choosing to disregard that standard, you also chose to break our downstream projects. I'm not sure what reaction you were expecting from that.

TL;DR The issue is not that you dropped support for a broken feature (because it's your own project and your own time), it's that you versioned the change in a way that had immediate negative impacts on others' projects.

Maybe that was the right choice for you, but that leaves me with the following questions, which I ask with all due respect and sincerity: