Skip to content

Change - to _ in build.board names #1341

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

Merged
merged 1 commit into from
May 14, 2018
Merged

Change - to _ in build.board names #1341

merged 1 commit into from
May 14, 2018

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Apr 20, 2018

build.board is used to define a macro but - is not a permitted character in macro names. This causes the macro name to actually be only everything up to the first - and also many warnings when compiling for one of these boards:

<command-line>:0:14: warning: ISO C++11 requires whitespace after the macro name

<command-line>:0:14: warning: ISO C99 requires whitespace after the macro name

I have followed the convention of the other build.board names by replacing - with _.

Related:
#812
#1187

build.board is used to define a macro but - is not a permitted character in macro names. This causes the macro name to actually be only everything up to the first - and also many warnings when compiling for one of these boards:

<command-line>:0:14: warning: ISO C++11 requires whitespace after the macro name

<command-line>:0:14: warning: ISO C99 requires whitespace after the macro name

I have followed the convention of the other build.board names by replacing - with _.
@per1234 per1234 changed the title Convert - to _ in build.board names Change - to _ in build.board names Apr 20, 2018
@MCUdude
Copy link

MCUdude commented Apr 21, 2018

YES, finally! Downloaded and installed arduino-esp32 last week, and the list of compile warnings was just overwhelming!

@reaper7
Copy link
Contributor

reaper7 commented Apr 21, 2018

I think that also variant name and corresponding to variant subfolder
should be changed from names with - to _
as is done for m5stack #1242

@per1234
Copy link
Contributor Author

per1234 commented Apr 21, 2018

the list of compile warnings was just overwhelming!

@MCUdude it seems there is the intention to avoid warnings in this project because they actually use -Werror=all when warnings are set to More or All in File > Preferences. That evidently doesn't upgrade preprocessor warnings to errors. The side effect of this is that warning level preference can affects whether code will compile. That's been the source of some confusion but it's for a good cause.

I think that also variant name and corresponding to variant subfolder
should be changed

I'm happy to do so if that's what the team wants.

The current build.variant value works fine since it determines a folder name and folder names may contain -, no problem. Changing the variant name was discussed in #812 and in that case it was decided that the build.variant value should not be changed and that decision has stood to this day.

The benefit of changing build.variant is purely for the sake of consistency with build.board but I really don't see that as a big thing since these two properties are used for completely different purposes.

The downside to the change is that it's harder to follow Git history through folder name changes. However, these variants only have one or two commits each so that's much of a problem. Certainly it's better to do it now than later.

@per1234
Copy link
Contributor Author

per1234 commented Apr 21, 2018

There is another possible downside to the variant name change I just thought of. It's possible to reference variants from another package (e.g. myboard.build.variant=espressif:esp32-gateway). So you could consider altering the name of a variant to be a breaking API change.

A Google search didn't find any packages that reference these variants so that breakage may be purely theoretical.

@reaper7
Copy link
Contributor

reaper7 commented Apr 21, 2018

it's just a suggestion...
You convinced me that it was pointless :)

@me-no-dev me-no-dev merged commit 25678f4 into espressif:master May 14, 2018
Curclamas pushed a commit to Curclamas/arduino-esp32 that referenced this pull request Aug 21, 2018
build.board is used to define a macro but - is not a permitted character in macro names. This causes the macro name to actually be only everything up to the first - and also many warnings when compiling for one of these boards:

<command-line>:0:14: warning: ISO C++11 requires whitespace after the macro name

<command-line>:0:14: warning: ISO C99 requires whitespace after the macro name

I have followed the convention of the other build.board names by replacing - with _.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants