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

Upgrade to pants 2.13 and remove sendwave-pants-docker dependency #16

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jeancochrane
Copy link
Contributor

This PR upgrades the plugin to the most recent stable version of pants (2.13).

As of pants 2.10, pants now has first-class support for Docker as a backend, so this PR also removes the dependency on our custom Docker plugin and refactors to use the pants-maintained backend.

Deliverance added 3 commits September 16, 2022 13:22
As of 2.10, this option is no longer necessary because pants sets it automatically based on the amount of concurrency available in the environment: https://www.pantsbuild.org/v2.10/docs/reference-python#section-resolver-jobs
@@ -1 +1 @@
python_requirements()
python_requirements(name="reqs")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find this in the changelog, but after updating to 2.13 pants started complaining that names are required for root-level targets:

$ ./pants dependencies ::
14:19:33.73 [ERROR] 1 Exception encountered:

  MappingError: Failed to parse ./BUILD:
Targets in root-level BUILD files must be named explicitly.

@@ -103,6 +108,10 @@ have reproducable builds, you may generate one by running:
#+END_SRC

* Changelog
* 1.2.0
TBD
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can set this when we're ready to cut a release! And if we want to use this PR for the release we can just add it via commit before merging.

Comment on lines -6 to -12
[python]
# Limit the maximum number of concurrent jobs used to resolve third
# party dependencies. The max level of parallelism will be
# `process_execution_local_parallelism x resolver_jobs`, but
# often you will have one resolve process at a time.
resolver_jobs = 2

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of 2.10, pants sets the number of concurrent jobs automatically based on the environment: https://www.pantsbuild.org/v2.10/docs/reference-python#section-resolver-jobs

Comment on lines +28 to +29
build_verbose = true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -255 to -263
@rule
async def node_project_docker(field_set: NodeProjectFieldSet) -> DockerComponent:
"""Build a node_package target into a DockerComponent.

This allows files generated by the node process to be included in
the docker image.
"""
package = await Get(Digest, NodeProjectFieldSet, field_set)
return DockerComponent(sources=package, commands=[])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent some time trying to replace this with built-in Docker components, but after deciding to try leaving it out it seems like it works. I think this is probably because the Docker backend builds the context automatically based on the target dependencies but I am not totally sure!

Comment on lines +28 to +38
search_paths = StrListOption(
"--search-paths",
default=["/bin", "/usr/bin/"],
help="Directories in which to search for node binaries.'",
)
use_nvm = BoolOption(
"--use-nvm",
default=True,
help="If true, the value of $NVM_BIN will be "
"appended to the front of the search path.",
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subsystem.register_options is now deprecated in favor of the more declarative Option syntax: https://github.com/pantsbuild/pants/blob/main/src/python/pants/notes/2.11.x.md#plugin-api-changes-1

name="docker_with_webpack_bundle",
instructions=[
"FROM python:3.9.1-slim-buster",
"COPY public /public",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not exactly the same as the previous source inference based on node_package.artifact_paths but it seems like perhaps it gets us where we need to go?

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.

1 participant