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

Add script to lint targets.json #4215

Merged
merged 7 commits into from Jul 7, 2017

Conversation

Projects
None yet
4 participants
@theotherjimmy
Contributor

theotherjimmy commented Apr 21, 2017

Description

The linting script for targets.json (tools/targets/lint.py) is a utility for
avoiding common pit-falls of defining targets, and detecting style
inconsistencies between targets.

Rules Enforced

A target's inheritance must look like one of these:

MCU -> Board
MCU -> Module -> Board
Family -> MCU -> Board
Family -> MCU -> Module -> Board
Family -> SubFamily -> MCU -> Board
Family -> SubFamily -> MCU -> Module -> Board

For each of these target roles, some rules are in place:

  1. For Family, MCU and SubFamily, the following keys are allowed
  • core
  • extra_labels
  • features
  • bootloader_supported
  • device_name
  • post_binary_hook
  • default_tool chain
  • public
  • config
  • target_overrides
  1. MCU's are required to have, and Families and SubFamilies may have:
  • release_versions
  • supported_toolchains
  • default_lib
  • device_has
  1. For Module and Board, the following keys are allowed:
  • supported_form_factors
  • is_disk_virtual
  • detect_code
  • extra_labels
  • public
  • config
  • forced_reset_timeout
  • target_overrides

macros is missing from this list. That is intentional: they do not provide any benefit over config and target_overrides but can be very difficult to use. In practice it is very difficult to override the value of a macro with a value. config on the other hand, was designed for this.

  • device_has may only contain values from the following list:
    • ANALOGIN.
    • ANALOGOUT.
    • CAN.
    • ETHERNET.
    • EMAC.
    • FLASH.
    • I2C.
    • I2CSLAVE.
    • I2C_ASYNCH.
    • INTERRUPTIN.
    • LOWPOWERTIMER.
    • PORTIN.
    • PORTINOUT.
    • PORTOUT.
    • PWMOUT.
    • RTC.
    • TRNG.
    • SERIAL.
    • SERIAL_ASYNCH.
    • SERIAL_FC.
    • SLEEP.
    • SPI.
    • SPI_ASYNCH.
    • SPISLAVE.

extra_labels may not contain any target names.

if release_versions contains 5, then supported_toolchains must contain all
of GCC_ARM, ARM and IAR.

Sample output

There are three commands, targets, all-targets and orphans. The targets
and all-targets commands both show errors within public inheritance
hierarchies. The orphans command shows all targets that are not reachable from
a public target.

python tools/targets/lint.py targets EFM32GG_STK3700 EFM32WG_STK3800 LPC11U24_301

hierarchy: Family (EFM32) -> MCU (EFM32GG990F1024) -> Board (EFM32GG_STK3700)
target errors:
  EFM32:
  - EFM32 is not allowed in extra_labels
  EFM32GG990F1024:
  - macros found, and is not allowed
  - default_lib not found, and is required
  - device_has not found, and is required
  EFM32GG_STK3700:
  - progen found, and is not allowed
  - device_has found, and is not allowed
---
hierarchy: Family (EFM32) -> MCU (EFM32WG990F256) -> Board (EFM32WG_STK3800)
target errors:
  EFM32:
  - EFM32 is not allowed in extra_labels
  EFM32WG990F256:
  - macros found, and is not allowed
  - default_lib not found, and is required
  - device_has not found, and is required
  EFM32WG_STK3800:
  - progen found, and is not allowed
  - device_has found, and is not allowed
---
hierarchy: Family (LPCTarget) -> MCU (LPC11U24_301) -> ???
hierarchy errors:
- no boards found in heirarchy
target errors:
  LPC11U24_301:
  - release_versions not found, and is required
  - default_lib not found, and is required
  - public not found, and is required

python tools/targets/lint.py orphans

- CM4_UARM
- CM4_ARM
- CM4F_UARM
- CM4F_ARM
- LPC1800
- EFR32MG1P132F256GM48
- EFR32MG1_BRD4150

TODO

  • Documentation
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 21, 2017

@0xc0170

The commit msg are brief compare to the description and importance of this patch?

@@ -0,0 +1,262 @@
"""A linting utility for targets.json

This comment has been minimized.

@0xc0170

0xc0170 Apr 24, 2017

Member

license header here

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 24, 2017

Contributor

Yep. Good catch.

This comment has been minimized.

@theotherjimmy

theotherjimmy May 18, 2017

Contributor

license header added.

tools/targets/lint.py Outdated
from copy import copy
from yaml import dump_all
import argparse
from tools.targets import Target, set_targets_json_location, TARGET_MAP

This comment has been minimized.

@0xc0170

0xc0170 Apr 24, 2017

Member

dont we do first local imports then system or other way around (anyway separated by an empty line) ?

This comment has been minimized.

@theotherjimmy

theotherjimmy Apr 24, 2017

Contributor

We do system then local (pylint prefers this style by default, and changing pylint's defaults seems bad). I'll add a newline.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 24, 2017

@theotherjimmy Where we can document these requirements? It should come with this lint addition. Macros are shown as an error but are not specified in the description - macros found, and is not allowed (to explain why macros cant be defined for MCU?).

hierarchy: Family (LPCTarget) -> MCU (LPC11U24_301) -> ???
hierarchy errors:

  • no boards found in heirarchy

How is this looked at, that LPC11U24_301 is not a board (in this case it is). I assume MCU -> Board. In this case it is just MCU (there's no board) ?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 24, 2017

How is this looked at, that LPC11U24_301 is not a board (in this case it is). I assume MCU -> Board. In this case it is just MCU (there's no board) ?

Well, It looks an awful lot like an MCU: it defines most of the things that an MCU would define, and almost none of the things that a board would define.

For reference:

    "LPC11U24_301": {
        "inherits": ["LPCTarget"],
        "core": "Cortex-M0",
        "extra_labels": ["NXP", "LPC11UXX"],
        "supported_toolchains": ["ARM", "uARM", "GCC_ARM", "IAR"],
        "device_has": ["ANALOGIN", "ERROR_PATTERN", "I2C", "I2CSLAVE", "INTERRUPTIN", "LOCALFILESYSTEM", "PORTIN", "PORTINOUT", "PORTOUT", "PWMOUT", "SEMIHOST", "SERIAL", "SLEEP", "SPI", "SPISLAVE", "STDIO_MESSAGES"],
        "device_name": "LPC11U24FHI33/301"
    },

You would expect an mcu to define core, supported_toolchains, device_has, and device_name.

This linter is based on a discussion that @sg- and I had offline relating to how we would like targets.json to be used. It does not reflect the current structure of targets.json.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 24, 2017

Where we can document these requirements? It should come with this lint addition.

Agreed. I'm typing them up and submitting the changes to the docs handbook. I'll make a note not to merge the docs until after this PR is merged and put onto a release.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 24, 2017

Macros are shown as an error but are not specified in the description to explain why macros cant be defined for MCU

So @sg- , I and #3123 came to the same conclusion: that macros are a bad mechanism for defining configuration, and that config should be preferred. We could not come up with a valid use for the macros parameter, so I did not turn it on.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 24, 2017

So @sg- , I and #3123 came to the same conclusion: that macros are a bad mechanism for defining configuration, and that config should be preferred. We could not come up with a valid use for the macros parameter, so I did not turn it on.

Agree, my point was that I could not find "why" in the description above or in the commit messages (please add that to the documentation).

Well, It looks an awful lot like an MCU: it defines most of the things that an MCU would define, and almost none of the things that a board would define.

I got an answer (my question was not that clear, I was aiming at "how linter knows if its MCU or board"). Based on the answer, there are requirements what MCU defines and what boards should and thus we get hierarchy: Family (LPCTarget) -> MCU (LPC11U24_301) -> ???.

Nice addition ! To have targets well-defined and a script to check - 👍 🍰

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Apr 24, 2017

Agree, my point was that I could not find "why" in the description above or in the commit messages (please add that to the documentation).

I'll add it to the description.

I got an answer (my question was not that clear, I was aiming at "how linter knows if its MCU or board"). Based on the answer, there are requirements what MCU defines and what boards should and thus we get

So the actual thing that happens is this: The linter looks for a transition point from "things that look like a board" to "things that look like an MCU" in the hierarchy, starting with the target. At each node (target, whataver), it checks the errors that would happen if it were both an MCU and a board. If there are more errors if it would be a board, then that's the transition point.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 2, 2017

bump @theotherjimmy get on this. geez

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 17, 2017

@theotherjimmy bump again...

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 18, 2017

@0xc0170 The docs and PR description have been updated to reflect the lack of macros.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 19, 2017

how shall we test this? When to run it (part of morph CI?) or just for every new target, or ?

@adbridge

This comment has been minimized.

Contributor

adbridge commented May 30, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented May 30, 2017

@0xc0170 We should have it run for every new target, and added in the comment thread.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 12, 2017

Does this need any other work ? Ready for being reviewed?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 12, 2017

It looks like it's ready for review.

@sg- sg- added needs: review and removed needs: work labels Jun 15, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 19, 2017

@theotherjimmy Get this review please.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

Just reviewed offline with @sg-. The commit came from that. Needs: work label added to update docs.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 27, 2017

Docs updated. PR description updated.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 28, 2017

To resolve jenkins CI, please rebase (there was a commit that is removing clientapp test for now that should avoid running it)

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Jun 28, 2017

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:lint-targets-json branch to 18bca08 Jun 29, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 29, 2017

@0xc0170 Rebased. Moving to Testing.

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels Jun 29, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 30, 2017

Will not be hit by any morph commands. Moving to ready for merge.

@adbridge adbridge merged commit d2cb0c6 into ARMmbed:master Jul 7, 2017

3 checks passed

Cam-CI uvisor Build & Test Success
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sg- sg- removed the ready for merge label Jul 7, 2017

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