chore: Improvements for local build process #5168
Conversation
The installation script creates a local .venv environment that needs to be activated before starting a build.
To exclude Python specific .venv folder
Updated the instructions to clarify the use of venv
mgyorke
force-pushed
the
chore/improvements-for-local-run
branch
from
66d97b0
to
ff634e0
Compare
mgyorke
force-pushed
the
chore/improvements-for-local-run
branch
from
ff634e0
to
8bd1ac9
Compare
| build-local: pre-build | ||
| @( \ | ||
| source $(WORKING_DIR)/.venv/bin/activate; \ | ||
| cd $(EXTENSIBLE_CLI_DIR); $(MAKE) build-full install bindir=$(WORKING_DIR)/$(BINARY_OUTPUT_FOLDER) USE_LEGACY_EXECUTABLE_NAME=1; \ |
There was a problem hiding this comment.
Question: Do we really need a differentiation between build and build-local? I'm worried that local and CI build get out of sync at some point. I would like to see everyone using the same command to build.
There was a problem hiding this comment.
Just to expand the thought, I wonder if we can make the usage of the virtual environment invisible to the make user.
There was a problem hiding this comment.
We could, the initial assumption in the MakeFile is that Python dependencies are installed globally before we run Make. This is handled by CircleCI in the preparatory steps.
We could change this, to use venv in the pipelines and then the build targets are the same everywhere. I see this as the ideal approach but I hesitated to touch the pipelines without discussing possible implications first.
The other two solutions I see are more pragmatic, as the current build path remains the same:
- dedicated build target for local build, proposed in this PR;
- do some checks in the existing build target so that it activates the venv if it runs locally or proceed as usual when running in container;
| @@ -37,14 +37,17 @@ make clean | |||
|
|
|||
| To build the project, run the following command in the root of the repository. | |||
|
|
|||
| Now we trigger the build process: | |||
There was a problem hiding this comment.
Question: Isn't this what the previous line says? Do you really want to keep both? Maybe merge their content?
There was a problem hiding this comment.
You're right, I'll clean this up once we decide on the approach.
| Another example, if we want the output in console and to run a single test suite by name: | ||
|
|
||
| ``` | ||
| npm run test:acceptance -- --selectProjects coreCli --reporters=default -t 'woof' |
There was a problem hiding this comment.
Suggestion: Let's remove the jest-junit from the package.json and only add it when running in CI. That way, the developer doesn't have to specify the default reporter, which improves the local developer experience.
| @@ -8,7 +8,7 @@ To install the required development dependencies in homebrew based environments, | |||
| The only additional prerequisite is having [homebrew](https://brew.sh/) installed. | |||
|
|
|||
| ```sh | |||
| ./scripts/install-dev-dependencies.sh | |||
| make install-deps | |||
There was a problem hiding this comment.
Question: can we assume make to be available? Probably since we don't install it anywhere, we might want to mention it like we do with brew above.
There was a problem hiding this comment.
I think so, AFAIK it's bunded with the Xcode Command line tools that are a prerequisite for homebrew.
We could check for the dependencies that we assume are already installed and inform the user.
I see how this may be necessary if the developer has different toolchains for managing dependencies and they touch PATH.
Pull Request Submission
Please check the boxes once done.
The pull request must:
feat:orfix:, others might be used in rare occasions as well, if there is no need to document the changes in the release notes. The changes or fixes should be described in detail in the commit message for the changelog & release notes.Pull Request Review
All pull requests must undergo a thorough review process before being merged.
The review process of the code PR should include code review, testing, and any necessary feedback or revisions.
Pull request reviews of functionality developed in other teams only review the given documentation and test reports.
Manual testing will not be performed by the reviewing team, and is the responsibility of the author of the PR.
For Node projects: It’s important to make sure changes in
package.jsonare also affectingpackage-lock.jsoncorrectly.If a dependency is not necessary, don’t add it.
When adding a new package as a dependency, make sure that the change is absolutely necessary. We would like to refrain from adding new dependencies when possible.
Documentation PRs in gitbook are reviewed by Snyk's content team. They will also advise on the best phrasing and structuring if needed.
Pull Request Approval
Once a pull request has been reviewed and all necessary revisions have been made, it is approved for merging into
the main codebase. The merging of the code PR is performed by the code owners, the merging of the documentation PR
by our content writers.
What does this PR do?
This PR simplifies the local development setup by automating dependency installation for Python and adding specific targets for local build.
Changes:
Dependency Management: Updated the install-dev-dependencies.sh script to create a .venv and install required Python dependencies at the project root.
Makefile Targets: Introduced two new Makefile targets:
make install-depsAutomates dependency setup, wraps the installation scriptmake build-localTriggers the local build process using the local .venvDocumentation: Updated CONTRIBUTING.md with clear instructions for local development setup and usage of the new targets.
Where should the reviewer start?
scripts/install-dev-dependencies.sh
MakeFile
How should this be manually tested?
make install-depsit shouldn't fail even if the dependencies are already installed.Check if the .venv is created and if it contains the dependencies.
make build-localit should prepare the build as usualAny background context you want to provide?
What are the relevant tickets?
Screenshots
Additional questions