Package build improvements: #181
Conversation
There was a problem hiding this comment.
A quick review (we will need another iteration after this). Some high-level comments as well:
- You're missing the validation section of the PR?
- You should add the build folder of each component to
.gitignore - The taskfile replaces the
packagerso we should ensure they generate the same output (from a user's perspective) and that we remove unnecessary files/docs, etc.
BUILDING.md
Outdated
| * [Meteor](https://docs.meteor.com/install.html#installation) | ||
| * [NodeJS 14](https://nodejs.org/download/release/v14.21.3/) |
There was a problem hiding this comment.
Meteor and NodeJS aren't requirements right now.
BUILDING.md
Outdated
| * Run `task` from the root of the project | ||
| * The package will be available at `out/clp-package` | ||
|
|
||
| NOTE: The taskfile is not set up perfectly so it's possible it may generate an |
There was a problem hiding this comment.
We should address this limitation in this PR.
BUILDING.md
Outdated
There was a problem hiding this comment.
This doc should be in docs/Building.md.
| [tool.poetry.group.dev.dependencies] | ||
| black = "^23.9.1" | ||
| docformatter = "^1.7.5" | ||
| mypy = "^1.5.1" | ||
| ruff = "^0.0.291" |
There was a problem hiding this comment.
Let's add these dev dependencies in another PR.
requirements.txt
Outdated
There was a problem hiding this comment.
Do we need these? Shouldn't this all be handled in the taskfile?
There was a problem hiding this comment.
We need poetry to build Python packages.
There was a problem hiding this comment.
Ah right, sorry, I forgot how the Taskfile was using this.
|
|
||
| [build-system] | ||
| requires = ["poetry-core"] | ||
| build-backend = "poetry.core.masonry.api" |
There was a problem hiding this comment.
Missing empty line at end of file.
Taskfile.yml
Outdated
| cmds: | ||
| - rm -rf {{.CLP_BUILD_DIR}}/clp-package | ||
|
|
||
| # TODO Clean-up |
There was a problem hiding this comment.
Should get rid of the TODO.
wraymo
force-pushed
the
package_prototype
branch
from
f1f2c15 to
7a52e01
Compare
There was a problem hiding this comment.
I have changes locally that I will commit once I clean them up, but in the mean time:
- You're still missing the validation section of the PR?
- For the Python components, we don't need the
constraints.txtandrequirements.txtfiles since we're now usingpyproject.toml - Similarly, in the Python component READMEs, the instructions should be about how to build and install with
poetry.
| @@ -0,0 +1,10 @@ | |||
| # CLP Package Utilities | |||
|
|
|||
| This python module contains utilities used by thegit CLP package. | |||
There was a problem hiding this comment.
| This python module contains utilities used by thegit CLP package. | |
| This python module contains utilities used by the CLP package. |
requirements.txt
Outdated
There was a problem hiding this comment.
To ensure reproducible builds, let's lock the versions of these packages.
wraymo
force-pushed
the
package_prototype
branch
from
79264ec to
4eb5bfd
Compare
…imestamps; Regenerate venv and wheel when requirements changes.
…nerate output when it's deleted.
wraymo
force-pushed
the
package_prototype
branch
from
20bbea3 to
7c66428
Compare
kirkrodrigues
changed the title
Description
The package is currently built using a Python script (the so-called "packager") which has no support for incremental builds. In addition, the way we install the Python components into the package is quite hacky and doesn't take advantage of Python's packaging tools.
This PR adds support for building the Python components using poetry and replaces the packager tool with a Taskfile (see Task). The Taskfile allows us to track changes to source files and only rebuild artifacts that depend on the changed source files.
One limitation of this PR (compared to the original packager) is that we haven't added support for automatically building the package in a container. As a result, users of this new build flow will need to setup a build machine/container manually. We hope to resolve this in a future PR.
Validation performed