Skip to content

Revert "Allow 'small-build' and 'big-build' to be used as options." #2568

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

Closed

Conversation

screamerbg
Copy link
Contributor

@screamerbg screamerbg commented Aug 29, 2016

Reverts #2409 as this breaks backwards compatibility with mbed 2.0 build tools and online build system. Namely the problem is in the "default_build" field which is being renamed to "default_lib".
As "default_lib" is mandatory attribute for all targets, the build system cannot be used with older targets.json files, like the ones released in revision 122 and 123 the mbed library (mbed 2.0 SDK), which have the "default_build" instead, e.g. https://developer.mbed.org/users/mbed_official/code/mbed/file/2241e3a39974/targets.json

My recommendation is to revert the field name to "default_build" and rather change how field values are handled.

@sg- @theotherjimmy @pan-

@theotherjimmy
Copy link
Contributor

Dang. That's a good point. I agree, the only way forward is to undo the name change.

@sg-
Copy link
Contributor

sg- commented Aug 29, 2016

@theotherjimmy or we can actually use the tools at the commit of the release. What do you think?

@theotherjimmy
Copy link
Contributor

@sg- That's a big of a can of worms that I don't want to open just yet.

@pan-
Copy link
Member

pan- commented Aug 30, 2016

@screamerbg Do you think it is desirable to let default_lib and default_build cohabit together for a while ?

@screamerbg
Copy link
Contributor Author

screamerbg commented Aug 30, 2016

@pan- wrote:
@screamerbg Do you think it is desirable to let default_lib and default_build cohabit together for a while ?

The original PR changed the data fields of targets.json in a non-backwards compatible way. Having them cohabit together makes no difference as once targets.json is fully migrated to use default_lib and default_build is dropped, the build system will crash with a python error if that the attribute is missing for a particular version of targets.json in mbed OS or the mbed library (e.g. rev 122 and 123).

@sg- wrote:
@theotherjimmy or we can actually use the tools at the commit of the release. What do you think?

Moving forward the plans is to execute the build tools directly from the mbed OS release itself (in online build system environment), though this still won't be compatible with the mbed library (2.0) releases in https://developer.mbed.org/users/mbed_official/code/mbed/. This means that we might have to bundle copy of the tools with the mbed library releases as well (note that targets.json is at a different location in the library releases so the location has to be adjusted or handled differently).

Generally, we should make careful considerations about new fields and structure changes in targets.json and attempt to reduce renames as much as possible as these are guaranteed (99.99%) to break backward compatibility. In this particular case, the simplest fix would be to make the python code accept both fields which means that the functional code of the build system will be carrying forward this legacy support (not desired). Also note that the PR changes the values as well, thus the python code would have to support 2 values for same setting (e.g. "std" and "standard").

Finally, "default_lib" sounds confusing. It's should be rather "default_clib" as the option controls the toolchain C library, not a generic library as "default_lib" naming implies.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 30, 2016

Generally, we should make careful considerations about new fields and structure changes in targets.json and attempt to reduce renames as much as possible as these are guaranteed (99.99%) to break backward compatibility. In this particular case, the simplest fix would be to make the python code accept both fields which means that the functional code of the build system will be carrying forward this legacy support (not desired). Also note that the PR changes the values as well, thus the python code would have to support 2 values for same setting (e.g. "std" and "standard").

+1.
As @pan- and @sg- asked above, I think that would be a good option to go forward, to have this change non-breaking (accept the old and new syntax / values), rather than reverting the entire change (it was accepted by few ppl to provide a new syntax and argument options). If that works nicely with the only build system (I assume it should). I'll talk to you in a bit about this revert

@theotherjimmy
Copy link
Contributor

Further integration of the new exporters is gated by this discussion/changeset.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 31, 2016

@screamerbg @sg- I've just rebased the release candidate branch and removed this default lib changeset from there. It's just on master, a chance to fit this changeset to the next release (non-breaking, backward compatible).

@sg-
Copy link
Contributor

sg- commented Sep 1, 2016

@pan- Can you make the update to re-add default_build??

@sg-
Copy link
Contributor

sg- commented Sep 22, 2016

@theotherjimmy can we just make these changes to the other repos?

-        "default_lib": "std"
+        "default_build": "standard"

@sg- sg- closed this Sep 28, 2016
@0xc0170 0xc0170 deleted the revert-2409-enable_small_and_big_build_options branch June 22, 2018 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants