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

Breaking change made without major version bump #5572

Closed
6 tasks done
per1234 opened this issue Jan 2, 2019 · 6 comments
Closed
6 tasks done

Breaking change made without major version bump #5572

per1234 opened this issue Jan 2, 2019 · 6 comments

Comments

@per1234
Copy link
Contributor

per1234 commented Jan 2, 2019

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: NA
  • Core Version: Since cc0bfa0
  • Development Env: NA
  • Operating System: NA

Settings in IDE

  • Module: NA
  • Flash Mode: NA
  • Flash Size: NA
  • lwip Variant: NA
  • Reset Method: NA
  • Flash Frequency: NA
  • CPU Frequency: NA
  • Upload Using: NA
  • Upload Speed: NA

Problem Description

boards.txt menu ID names were changed in cc0bfa0:

  • UploadSpeed -> baud
  • CpuFrequency -> xtal
  • FlashSize -> eesz
  • Debug -> dbg
  • DebugLevel -> lvl
  • LwIPVariant -> ip
  • VTable -> vt
  • FlashErase -> wipe

Changed menu option IDs:

  • eesz
    • 512K0 -> 512K
    • 1M0 -> 1M
  • ip
    • v2mss536 -> lm2
    • v2mss1460 -> hb2
    • Prebuilt -> hb1
    • OpenSource -> src

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
Copy link
Collaborator

d-a-v commented Jan 3, 2019

As you are mentionning it, it is a workaround to a windows limitation with the arduino builder which is now fixed by arduino/arduino-builder@ab41198

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
Copy link
Contributor Author

per1234 commented Jan 3, 2019

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. arduino/arduino-cli#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 cc0bfa0 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
Copy link
Collaborator

devyte commented Jan 3, 2019

@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
Copy link
Contributor Author

per1234 commented Jan 3, 2019

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
Copy link
Collaborator

devyte commented Jan 4, 2019

@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.

@devyte devyte added this to the 2.5.0 milestone Jan 4, 2019
@devyte devyte self-assigned this Jan 4, 2019
maciekn added a commit to maciekn/vscode-esp8266fs that referenced this issue Mar 9, 2019
maciekn added a commit to maciekn/vscode-esp8266fs that referenced this issue Mar 13, 2019
bxparks added a commit to bxparks/AceTime that referenced this issue May 10, 2019
bxparks added a commit to bxparks/AceRoutine that referenced this issue May 10, 2019
bxparks added a commit to bxparks/AUnit that referenced this issue May 10, 2019
@ianfixes
Copy link

ianfixes commented Dec 28, 2020

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:

  • What would have been the additional cost to you (in terms of time or effort) in releasing your breaking change as 3.0.0 instead of 2.4.0?
  • Why use semver at all if your own docs say you won't adhere to it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants