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

Add ZAP into third_party #3473

Merged
merged 5 commits into from
Oct 28, 2020

Conversation

andy31415
Copy link
Contributor

@andy31415 andy31415 commented Oct 27, 2020

Problem

ZAP will be used for code generation. Add it to third_party and ensure docker can do 'npm install' on it.

Summary of Changes

  • Adds zap into third_party
  • Adds the LTS version of nodejs into the dockerfile image
  • Adds dev prerequisites for a npm install of zap (required cairo and pango). resorted package list while I was changing the file to make it look nicer.

@jepenven-silabs
Copy link
Contributor

Perfect timing!

@rwalker-apple
Copy link
Contributor

@rwalker-apple
Copy link
Contributor

@mspang?

@mspang
Copy link
Contributor

mspang commented Oct 27, 2020

Problem

ZAP will be used for code generation. Add it to third_party and ensure docker can do 'npm install' on it.

Summary of Changes

  • Adds zap into third_party
  • Adds the LTS version of nodejs into the dockerfile image
  • required cairo and pango

Why do we need graphics/fonts libraries to build CHIP?

@mspang mspang requested a review from mrjerryjohns October 27, 2020 19:29
@andy31415
Copy link
Contributor Author

Problem

ZAP will be used for code generation. Add it to third_party and ensure docker can do 'npm install' on it.

Summary of Changes

  • Adds zap into third_party
  • Adds the LTS version of nodejs into the dockerfile image
  • required cairo and pango

Why do we need graphics/fonts libraries to build CHIP?

I believe we need to clean up. For now for ZAP, it is a single package under npm install so it brings in stuff to build the UI.

Chip should not need them, so I am thinking of alternatives like:

  • can we make the generator not use node (python? handlebars is reasonably standard)
  • can we make 'executable' for the generator somehow, so we install it like a binary in the image instead of putting repo in third-party?

Unsure what would be possible, for now assuming we may want to run the full zap until we make it smaller.

@vivien-apple
Copy link
Contributor

Problem

ZAP will be used for code generation. Add it to third_party and ensure docker can do 'npm install' on it.

Summary of Changes

  • Adds zap into third_party
  • Adds the LTS version of nodejs into the dockerfile image
  • required cairo and pango

Why do we need graphics/fonts libraries to build CHIP?

Basically ZAP is an Electron application. So it not a pure command line utility as it also contains a graphic UI.
That said it can be run as a command line application from the build system too since it rely on nodejs.

@mspang
Copy link
Contributor

mspang commented Oct 27, 2020

Bringing in node.js is going to cause extra work for us; we're not deploying that already and we'll have to figure out how to manage it.

We can afford that cost, but are we adding too much burden on the other project participants? Who else is already deploying node.js on their infrastructure?

Right now, to compile CHIP you minimally need

  • C++ toolchain
  • GN
  • ninja
  • python 3

That's pretty much it. There are some other dependencies depending on platform, but those are related to particular ports not CHIP itself. Furthermore, if you'd rather not spend time managing dependencies we'll actually ship you all of those things on Linux, Mac, & Windows (that's what the "bootstrap" step does).

[we did drop bundling python3 on Linux recently - we may want to bring it back, per slack, doing so already broke someone's build]

If everyone is comfortable with this, OK, but I think we should be careful here.

@gerickson
Copy link
Contributor

+1 to @mspang's comments. We should keep the dependencies on what it takes to build the "common core" of CHIP (whatever we want to bill that as) as minimal and simple as possible. Node.js seems to go one step too far. If it's for a particular binding, then we should either start forecasting a CHIP packaging / repo approach that layers onto or shells around "common core" where those packages / repo are optional adjuncts.

@mrjerryjohns
Copy link
Contributor

mrjerryjohns commented Oct 27, 2020

I'd concur. The code generator has to be lean, with minimal dependencies given its need to run on a multitude of different platforms, use-cases. Requiring node.js seems like a very heavy lift, and will make it much less usable, which if it wasn't a critical tool, I'd have cared less. But the code generator is a must-have tool for anyone who intends to interact with CHIP devices.

@bzbarsky-apple
Copy link
Contributor

My understanding is that using ZAP as the code generator was the plan of record starting at the point when the Silicon Labs ZCL implementation was selected for integration into CHIP.

What are the alternatives people are proposing, precisely? As I see it, we have the following options:

  • Use ZAP, as planned. This does add a node.js dependency to CHIP builds, which I agree is not nearly as nice as not adding a dependency. That said, in practical terms, the costs here are setting up CHIP CI and the cost to people actually using CHIP. Do we have an estimate for the former? Is the latter a serious bar? This is needed at build-time, not runtime, so it's not clear to me how wide a variety of platforms we really need the generator to support in practice...

  • Write a different generator (presumably in python?) that consumes the output of the ZAP UI and generates C++ code. Timeframe on doing this? How much of the work here would duplicate ZAP work that's happening anyway vs being CHIP-specific?

  • Figure out whether there's an existing python package that allows using as much of the existing ZAP bits (templates, input data, etc) as possible. Not sure whether in practice this would be any less work than "write a different generator".

Any other constructive suggestions?

Note that the long-term tradeoffs here might be different from the "get to 1.0 by a certain date" ones, by the way; it's a lot easier for me to see us writing a separate python-based generator long-term than in the next few months....

@gerickson
Copy link
Contributor

My understanding is that using ZAP as the code generator was the plan of record starting at the point when the Silicon Labs ZCL implementation was selected for integration into CHIP.

What are the alternatives people are proposing, precisely? As I see it, we have the following options:

  • Use ZAP, as planned. This does add a node.js dependency to CHIP builds, which I agree is not nearly as nice as not adding a dependency. That said, in practical terms, the costs here are setting up CHIP CI and the cost to people actually using CHIP. Do we have an estimate for the former? Is the latter a serious bar? This is needed at build-time, not runtime, so it's not clear to me how wide a variety of platforms we really need the generator to support in practice...
  • Write a different generator (presumably in python?) that consumes the output of the ZAP UI and generates C++ code. Timeframe on doing this? How much of the work here would duplicate ZAP work that's happening anyway vs being CHIP-specific?
  • Figure out whether there's an existing python package that allows using as much of the existing ZAP bits (templates, input data, etc) as possible. Not sure whether in practice this would be any less work than "write a different generator".

Any other constructive suggestions?

Note that the long-term tradeoffs here might be different from the "get to 1.0 by a certain date" ones, by the way; it's a lot easier for me to see us writing a separate python-based generator long-term than in the next few months....

Fair points all and if there's a roadmap, then this might be an easier short-term "get it done" pill to swallow. What's the long-term plan for the Data Model Work Group? The One Data Model effort was/is heading towards SDF. Is that where this post-1.0 and the DMWG efforts are headed? What are SDF's tooling requirements?

@woody-apple
Copy link
Contributor

woody-apple commented Oct 27, 2020

FYI: Electron has been the plan of record since the original Data Model source code evaluation, when we discussed the code from Silicon Labs. The code generator can run as a nodejs service, that will take far less time to run than most steps of our build phase itself.

Edit: GN setup currently takes 2 minutes to configure itself, and just under 2 minutes to build. I'm doubtful that this nodejs service will take much compute time to run. If npm, nodejs, and dups are installed in the build container already

@bzbarsky-apple
Copy link
Contributor

@gerickson I'm not privy to the long-term Data Model Work Group plans. That said, there is a distinction between the metadata that is input into generator, and which is presumably largely defined by the spec, and the code templates which the generator then uses and how it composes them based on the metadata, which is likely to be somewhat CHIP-specific. I don't know that there is any credible plan I've seen for sharing a code generator across CHIP and ZigbeePro, if that's what you were asking.

@hawk248
Copy link

hawk248 commented Oct 27, 2020

Folks - node.js as a runtime requirement would give me reservations ... as a CI or build tool does not seem too onerous. Therefore I am inclined to support its inclusion as proposed.

@rwalker-apple
Copy link
Contributor

rwalker-apple commented Oct 28, 2020

weighing in (late):

ZAP as-is is (like the ZCL impl we agreed to and our huge examples tree) currently a necessary evil. These 2 projects need to move lockstep during this dev-heavy phase.

Boiling ZAP down to something we can install as a flask or as a debian package would improve the situation greatly.

Backing out handlebars and using mustache for bash would be my preference, though ;)

@rwalker-apple
Copy link
Contributor

Folks - node.js as a runtime requirement would give me reservations ... as a CI or build tool does not seem too onerous. Therefore I am inclined to support its inclusion as proposed.

@hawk248 node.js would be a requirement for accessory devs, does that change anything for you?

@rwalker-apple
Copy link
Contributor

rwalker-apple commented Oct 28, 2020

Pushing button. Let's think about how ZAP's dependency footprint can be reduced, how we can make zapc XXX.zap -o gen be as painless as possible

@rwalker-apple rwalker-apple merged commit 3bd1a07 into project-chip:master Oct 28, 2020
@andy31415 andy31415 deleted the 01_zap_integration branch October 28, 2020 15:07
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.