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

fix: gradle builds #3250

Merged

Conversation

ramonpetgrave64
Copy link
Collaborator

@ramonpetgrave64 ramonpetgrave64 commented Feb 9, 2024

Fixes the Gradle builds #2727

I think the first attempt to fix (now reverted) was mostly correct, but in this PR I correct the directory comparison conditional.

Also adds some documentation for handling multi-project builds, which seem to now be the default when initializing a new Gradle app.

Testing

Tested against my own sample project

Modified the slsa-framwork/example-package e2e tests against my own fork. The actual builds and provenance generation succeed, except for the verify stage, which should fail because my fork https://github.com/ramonpetgrave64/slsa-github-generator/.github/workflows/builder_gradle_slsa3.yml@refs/heads/main is not a "trusted builder".

This reverts commit b8fdd83.

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
@ramonpetgrave64
Copy link
Collaborator Author

Copy link
Collaborator

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

Thanks. Can you send a PR. to slsa-framework/example-packages with a new workflow to test this fix?

internal/builders/gradle/README.md Outdated Show resolved Hide resolved
internal/builders/gradle/action.yml Outdated Show resolved Hide resolved
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
Signed-off-by: Ramon Petgrave <32398091+ramonpetgrave64@users.noreply.github.com>
@loosebazooka
Copy link
Contributor

it seems like reusing the build directory (a gradle concept) is a bit odd here? What we select as the slsa artifacts directory is arbitrary (and this isn't this PRs issue really). I guess the question is what should be adding to the slsa artifacts directory? Everything that the build used to build the final artifact? or just the final artifact?

@ramonpetgrave64
Copy link
Collaborator Author

Thanks. Can you send a PR. to slsa-framework/example-packages with a new workflow to test this fix?

Can you explain a bit more what you mean? You want me to submit and merge a PR that has a new workflow name that references my fork of slsa-github-generator?

@ramonpetgrave64
Copy link
Collaborator Author

@loosebazooka I also thought it odd to only generate provenance for a select few files of all the files that are served as artifacts. But for this PR I thought to not break existing working behavior.

@ianlewis
Copy link
Member

Thanks. Can you send a PR. to slsa-framework/example-packages with a new workflow to test this fix?

Can you explain a bit more what you mean? You want me to submit and merge a PR that has a new workflow name that references my fork of slsa-github-generator?

The example-packages repo is used for our e2e tests for all of the workflows in slsa-github-generator. They run on a schedule daily (I'm sure you've seen the issues that get created by them). Adding a new test here means creating a new workflow file in https://github.com/slsa-framework/example-package/tree/main/.github/workflows which is of the form e2e.<builder name>.<GHA event>.<branch name>.<label>.slsa3.yml. So for example, e2e.generic.push.main.upload-tag-name.slsa3.yml.

In this case something like e2e.maven.push.main.same-dir.slsa3.yml and e2e.gradle.push.main.same-dir.slsa3.yml might be appropriate. There is a bit of detail in how these work that you may have to grok by reading them (e.g. for testing push events, the e2e test workflows are triggered by a schedule, but then "bootstrap" the push event by pushing a new commit).

The test should reference the slsa-github-generator repo and be able to pass after this PR is merged. So adding the test after this PR is merged is fine.

Modified the slsa-framwork/example-package e2e tests against my own fork

In general, the tests in example-package shouldn't reference developer forks (I think there is one that @laurentsimon has that tests a specific scenario but that's an exception). If you need to test against your fork, you can do that with a test repository on your account. For example, here is one I use for testing: https://github.com/ianlewis/actions-test/tree/main/.github/workflows

@ramonpetgrave64
Copy link
Collaborator Author

@ianlewis So you're saying you want a new "pre-submit" workflow that runs as a PR Check, where the new workflow is meant to check the gradle and maven functionality?

@ianlewis
Copy link
Member

@ianlewis So you're saying you want a new "pre-submit" workflow that runs as a PR Check, where the new workflow is meant to check the gradle and maven functionality?

It's not a "pre-submit" since it won't block PRs. The e2e test workflows in example-package run daily on a schedule and create issues in this repo when the workflow fails. I believe that @laurentsimon means that we need to create a new workflow that checks the functionality you are fixing in this PR. We won't need it until after the PR is merged, so I think you can just note it, and not take it as a PR blocker.

@ramonpetgrave64
Copy link
Collaborator Author

ramonpetgrave64 commented Feb 15, 2024

@ianlewis Ok I understand now. But the only way to test this fix properly in exaple-package is to make the gradle "project" directory also be the root of the repository. noted in the issue #2727 (comment)

@laurentsimon
Copy link
Collaborator

I'll wait until you have verified that changes fix the problems by testing your fork. Please comment when you've verified it works

@ramonpetgrave64
Copy link
Collaborator Author

@laurentsimon I did actually test with my own of both slsa-github-generator and example-package

my fork of slsa-github-generator

running the original e2e of example-package with my fork of slsa-github-generator

running the builder with my own sample gradle project with my own fork slsa-github-generator

@ianlewis
Copy link
Member

Ok I understand now. But the only way to test this fix properly in exaple-package is to make the gradle "project" directory also be the root of the repository. noted in the issue #2727 (comment)

I'm not sure about the exact problem, but if directory structure is an issue, you can create a copy of the gradle project in example-package specifically for your test.

@laurentsimon
Copy link
Collaborator

laurentsimon commented Feb 16, 2024

Ok I understand now. But the only way to test this fix properly in exaple-package is to make the gradle "project" directory also be the root of the repository. noted in the issue #2727 (comment)

I'm not sure about the exact problem, but if directory structure is an issue, you can create a copy of the gradle project in example-package specifically for your test.

yes, that's the next step after merging this P. Seems like you've done due diligence, so let's merge this PR. Please send a pr to example-package to close the loop and close #3255

@laurentsimon laurentsimon merged commit b097318 into slsa-framework:main Feb 16, 2024
75 checks passed
ramonpetgrave64 added a commit that referenced this pull request Mar 20, 2024
ramonpetgrave64 added a commit that referenced this pull request Mar 20, 2024
ramonpetgrave64 added a commit that referenced this pull request Mar 20, 2024
ramonpetgrave64 added a commit that referenced this pull request Mar 20, 2024
This reverts commit b097318.

Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
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.

None yet

4 participants