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.

Sign up for GitHub

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

chore: Improvements for local build process #5168

Closed
wants to merge 6 commits into from
Closed

chore: Improvements for local build process #5168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
19c1a24
chore: handle python dependencies in install-dev-dependencies.sh
mgyorke Apr 16, 2024
48edf83
chore: update .gitignore
mgyorke Apr 16, 2024
666750d
chore: add clarifications to CONTRIBUTING.md
mgyorke Apr 16, 2024
ebcbe3f
chore: move dependencies in requirements.txt
mgyorke Apr 17, 2024
33b0cd7
chore: added local build specific targets
mgyorke Apr 17, 2024
8bd1ac9
chore: instructions for using make target to install deps
mgyorke Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ __outputs__
/ts-binary-wrapper/README.md
/ts-binary-wrapper/SECURITY.md
/ts-binary-wrapper/src/generated
.venv
junit.xml

# Diagnostic reports (https://nodejs.org/api/report.html)
report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
Expand Down
19 changes: 15 additions & 4 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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 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.

```

## Setting up
Expand All @@ -17,7 +17,7 @@ Clone this repository with git.

```sh
git clone [email protected]/snyk/cli.git
cd snyk
cd cli
```

You will now be on our `main` branch. You should never commit to this branch, but you should keep it up-to-date to ensure you have the latest changes.
Expand All @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Isn't this what the previous line says? Do you really want to keep both? Maybe merge their content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll clean this up once we decide on the approach.


```sh
make build
make build-local
```

Run the build binary like this.
Run the build binary like this, depending on your architecture:

```sh
./binary-releases/snyk-macos --version
./binary-releases/snyk-macos-arm64 --version
```

## Running tests
Expand Down Expand Up @@ -116,6 +119,14 @@ You can run acceptance tests with:
npm run test:acceptance -- --selectProjects coreCli
```

The output is saved as _junit.xml_ inside the root folder.

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'
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

```

### Smoke Tests

Smoke tests typically don't run on branches unless the branch is specifically prefixed with `smoke/`. They usually run on an hourly basis against the latest published version of the CLI.
Expand Down
13 changes: 13 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,3 +288,16 @@ require-args:
ifndef ARGS
$(error cannot run: ARGS is undefined)
endif

# targets responsible for local dev environment
# install local dependencies
install-deps:
@bash $(WORKING_DIR)/scripts/install-dev-dependencies.sh

# trigger build using the local .venv for python
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; \
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to expand the thought, I wonder if we can make the usage of the virtual environment invisible to the make user.

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 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;

)
$(MAKE) clean-package-files
8 changes: 8 additions & 0 deletions scripts/install-dev-dependencies.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,12 @@
set -exuo pipefail

# requires https://brew.sh/
# install dev dependencies from homebrew
brew bundle --file=$(dirname "$0")/Brewfile

# create python venv and activate it
python3 -m venv .venv
source .venv/bin/activate

# install dependencies
pip install -r scripts/requirements.txt
6 changes: 6 additions & 0 deletions scripts/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
certifi==2024.2.2
charset-normalizer==3.3.2
idna==3.7
PyYAML==6.0.1
requests==2.31.0
urllib3==2.2.1
Loading