move python side of workbench to the invest repo #72
Conversation
…s instead of building from source
|
Oh and there's a related invest PR that it probably makes sense to review kind of in parallel. natcap/invest#399 Probably neither should be merged until we're happy with both being merged. |
| @@ -6,13 +6,14 @@ | |||
| "main": "src/main.js", | |||
| "homepage": "./", | |||
| "scripts": { | |||
| "start": "npx electron . --dev", | |||
There was a problem hiding this comment.
Was the reason for dropping npx just because it's not really needed?
There was a problem hiding this comment.
Yeah, npx was pretty redundant inside this "scripts" context. I mainly use npx because it searches the local node_modules first for CLIs like electron, etc. But the "scripts" context already ensures that, I've since learned.
readme.md
Outdated
| ## To build this application | ||
| For development, choose either: | ||
| **A.** Duplicate the production setup by fetching prebuilt binaries `npm run fetch-invest` | ||
| **B.** Use an any other locally installed, compatible, invest CLI (e.g. from a local python environment). To configure this, see `.env-example` |
| * build invest binaries | ||
| `python -m PyInstaller --workpath build/pyi-build --clean --distpath build ./invest-flask.spec` | ||
| * `npm start` | ||
| * bind to an `invest` executeable (see package.json "invest" for a compatible version) |
There was a problem hiding this comment.
Related to this and invest in package.json, is this something that developers are expected to change frequently and commit in PRs?
There was a problem hiding this comment.
I don't think it will be changed frequenty, but yes, the "invest" property will need to be updated to whatever version of invest we want packaged with the workbench. Maybe in the future we would want to programmaticaly default to the latest invest release, but for now hardcoding the invest version in package.json seems okay to me.
| prefix commands with `npx` (e.g. `npx eslint ...`). Otherwise, only | ||
| globally installed packages are on the PATH. |
There was a problem hiding this comment.
Hey @davemfish This seems like a good move to me overall and I only had a few comments / questions. My main question / concern was around the invest setup in package.json and how this will work in practice for development and PRs. Should we also be providing a link those archives in the Readme from which to grab InVEST binaries from?
I'll add more thoughts on the InVEST side of things!
|
This PR now also fixes #73 |
|
Looks like the mac test is failing to download the binaries, maybe a wrong url? |
Right, this should be due to the fact that the invest Mac builds failed on the invest commit this build is linked to. |
This PR fixes #70 and fixes #71
The main changes are to the build process. The workbench build process will no longer build invest binaries from source, instead there is a
npm run fetch-investstep that will look for pre-built binaries on GCS to download and bundle with the electron distribution.package.jsonnow has an "invest" property to configure which invest binaries will be fetched for workbench builds.The flask app that was formerly
server.pywill now live atnatcap.invest.web_apiand can be launched from the invest CLI (invest serve). That means the workbench only needs to include that one python executeable (the invest cli) when it formerly needed two. Those changes are evident inmain_helpers.js.This PR also drops binary builds for ubuntu - since we don't make invest binaries for ubuntu. And it skips the puppeteer test on macos because they reliably don't work at the moment (see #52 ).