Skip to content

chore: Makefile tweaks #841

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

Closed
wants to merge 8 commits into from
Closed

chore: Makefile tweaks #841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2b0a557
build: .venv and gradle + java warnings
tinvaan Aug 20, 2024
22a904b
docs: fix java version warning when installing Gradle
tinvaan Aug 21, 2024
1810db5
chore: ignore repo finder mocks and jdk artifacts
tinvaan Aug 21, 2024
751c39c
chore: error out of java version is invalid
tinvaan Aug 21, 2024
09c5fb6
build: provide option to skip Souffle installs to unblock build
tinvaan Aug 21, 2024
b145015
build: silence checks and make pre-commit hook installation optional
tinvaan Aug 21, 2024
4f634f2
build: fix virtual env dir check
tinvaan Aug 23, 2024
85446ff
build: chore fallback to JDK 17
tinvaan Aug 23, 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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ tests/slsa_analyzer/build_tool/mock_repos/gradle_repos/no_gradle/
tests/slsa_analyzer/build_tool/mock_repos/maven_repos/no_pom/
tests/slsa_analyzer/checks/mock_repos/**
tests/slsa_analyzer/ci_service/mock_repos/**
tests/repo_finder/mock_repos/**
docs/_build
bin/
requirements.txt
Expand Down
38 changes: 31 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,11 @@ venv:
# install SLSA verifier binary, download mvnw, and gradlew.
.PHONY: setup
setup: force-upgrade setup-go setup-binaries setup-schemastore
ifeq ($(PRE_COMMIT_HOOKS),false)
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to make pre-commit optional? Our development environment requires pre-commit hooks.

Copy link
Author

Choose a reason for hiding this comment

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

This is purely for dev convenience & is also related to the 3rd party(Souffle) build break.

make setup triggers the pre-commit hooks and because tests fail (as expected), build/setup step is blocked as a result.

$(warning "Pre-commit hooks were not installed. Install manually using `pre-commit install` command.")
else
pre-commit install
endif
mkdir -p dist
go install github.com/CycloneDX/cyclonedx-gomod/cmd/cyclonedx-gomod@v1.3.0
setup-go:
Expand All @@ -107,13 +111,17 @@ $(PACKAGE_PATH)/resources/mvnw:
&& echo -e "distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.8.6/apache-maven-3.8.6-bin.zip\nwrapperUrl=https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.1.1/maven-wrapper-3.1.1.jar" > .mvn/wrapper/maven-wrapper.properties \
&& cd $(REPO_PATH)
$(PACKAGE_PATH)/resources/gradlew:
ifeq ($(shell java --version 2>&1 | grep '17.' >/dev/null; echo $$?), 0)
cd $(PACKAGE_PATH)/resources \
&& export GRADLE_VERSION=7.6 \
&& wget https://services.gradle.org/distributions/gradle-$$GRADLE_VERSION-bin.zip \
&& unzip -o gradle-$$GRADLE_VERSION-bin.zip \
&& rm -r gradle-$$GRADLE_VERSION-bin.zip \
&& gradle-$$GRADLE_VERSION/bin/gradle wrapper \
&& cd $(REPO_PATH)
else
$(error Java 17 is required for Gradle 7.6 installation. See the Grable compatbility matrix: https://docs.gradle.org/current/userguide/compatibility.html)
endif
setup-schemastore: $(PACKAGE_PATH)/resources/schemastore/github-workflow.json $(PACKAGE_PATH)/resources/schemastore/LICENSE $(PACKAGE_PATH)/resources/schemastore/NOTICE
$(PACKAGE_PATH)/resources/schemastore/github-workflow.json:
cd $(PACKAGE_PATH)/resources \
Expand Down Expand Up @@ -145,7 +153,10 @@ else
endif
.PHONY: souffle
souffle:
if ! command -v souffle; then \
ifeq ($(INSTALL_SOUFFLE),false)
$(warning "Skipping Souffle installation. Set INSTALL_SOUFFLE=true to install.")
Copy link
Member

Choose a reason for hiding this comment

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

I assume you are trying to optionally disable the Souffle installation, but the implementation below should have handled that. Did you get this error on your macOS?

Copy link
Author

Choose a reason for hiding this comment

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

No, the brew command failed for me instead. AFAIK, command -v <tool> only checks if <tool> exists in the system path. My issue is that Souffle's homebrew package's M* OSX build is broken.

else
@if ! command -v souffle; then \
echo "Installing system dependency: souffle" && \
case $(OS_DISTRO) in \
"Oracle Linux") \
Expand All @@ -166,10 +177,9 @@ souffle:
else \
echo "Unable to install Souffle. Please install it manually." && exit 0; \
fi ;; \
esac; \
fi && \
command -v souffle || true

esac; \
fi
endif

# Install or upgrade an existing virtual environment based on the
# package dependencies declared in pyproject.toml.
Expand All @@ -188,10 +198,24 @@ upgrade: .venv/upgraded-on
python -m pip install --upgrade --upgrade-strategy eager --editable .[actions,dev,docs,hooks,test,test-docker]
$(MAKE) upgrade-quiet
force-upgrade:
rm -f .venv/upgraded-on
@if [ -f .venv/upgraded-on ]; then \
rm -f .venv/upgraded-on; \
else \
rm -f ${VIRTUAL_ENV}/upgraded-on; \
Copy link
Member

Choose a reason for hiding this comment

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

Can you please provide more details? What was the original issue?

Copy link
Author

@tinvaan tinvaan Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll add more details in the PR description but TL;DR - if a user is using a Python version manager such as pyenv to manage virtual envs, the make script fails because it's looking for a hardcoded file that's probably specific to venv.

pyenv for instance sets the VIRTUAL_ENV variable so appending the upgraded-on file to it's path fixes the breakage.

$ echo $VIRTUAL_ENV
   ... <nothing here> ...

$ pyenv activate oracle@macaron
   ... <nothing here> ...

(oracle@macaron) $ echo $VIRTUAL_ENV
/Users/harish/.pyenv/versions/3.11.9/envs/oracle@macaron

fi

$(MAKE) upgrade
upgrade-quiet:
echo "Automatically generated by Python Package Makefile on $$(date '+%Y-%m-%d %H:%M:%S %z')." > .venv/upgraded-on
@if [ -d .venv/ ]; then \
echo "Automatically generated by Python Package Makefile for macaron on $$(date '+%Y-%m-%d %H:%M:%S %z')." > .venv/upgraded-on; \
else \
if [ ! -z "$(VIRTUAL_ENV)" ] && [ -d $(VIRTUAL_ENV) ]; then \
echo "Automatically generated by Python Package Makefile for macaron on $$(date '+%Y-%m-%d %H:%M:%S %z')." > $(VIRTUAL_ENV)/upgraded-on; \
else \
echo Failed to upgrade. Python virtual environment not found.; \
fi \
fi;

upgrade-go:
go get $$(go list -f '{{if not (or .Main .Indirect)}}{{.Path}}{{end}}' -m all)
go mod tidy
Expand Down
4 changes: 4 additions & 0 deletions src/macaron/resources/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,7 @@ build

# Ignore downloaded schemastore files
schemastore

# Any JDK artifacts downloaded by the user
jdk-*.jdk
jdk-*.tar.gz
Loading