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

Make forbidden apis check cacheable and cc compatible #101217

Merged
merged 4 commits into from
Nov 1, 2023

Conversation

breskeby
Copy link
Contributor

@breskeby breskeby commented Oct 23, 2023

Also ported the implementation to use the gradle worker api that allows us to expand our parallalism of task execution within a single project. Example build scan: https://gradle-enterprise.elastic.co/s/non7hf532jrlg/timeline?sort=longest&type=org.elasticsearch.gradle.internal.precommit.CheckForbiddenApisTask

@breskeby breskeby self-assigned this Oct 23, 2023
@breskeby breskeby added >enhancement :Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team labels Oct 23, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@breskeby breskeby marked this pull request as draft October 23, 2023 15:40
@breskeby breskeby force-pushed the improve-forbidden-apis-checks branch from 6a1f6e8 to 8ddbfee Compare October 23, 2023 17:11
@breskeby breskeby force-pushed the improve-forbidden-apis-checks branch from f0c6a65 to 6c374c6 Compare October 24, 2023 13:28
@@ -88,8 +88,6 @@ public class DependencyLicensesTask extends DefaultTask {

private final Logger logger = Logging.getLogger(getClass());

private static final String SHA_EXTENSION = ".sha1";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not used; removed while been there

@breskeby breskeby marked this pull request as ready for review October 24, 2023 13:29
@breskeby breskeby requested a review from a team October 24, 2023 13:29
@@ -65,60 +52,36 @@ public TaskProvider<? extends Task> createTask(Project project) {
String sourceSetTaskName = sourceSet.getTaskName(FORBIDDEN_APIS_TASK_NAME, null);
var sourceSetTask = project.getTasks().register(sourceSetTaskName, CheckForbiddenApisTask.class, t -> {
t.setDescription("Runs forbidden-apis checks on '${sourceSet.name}' classes.");
t.setResourcesDir(resourcesDir);
t.getOutputs().upToDateWhen(Specs.SATISFIES_ALL);
t.setClassesDirs(sourceSet.getOutput().getClassesDirs());
t.dependsOn(resourcesTask);
t.setClasspath(sourceSet.getRuntimeClasspath().plus(sourceSet.getCompileClasspath()).plus(sourceSet.getOutput()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why all the plus nonsense here? Shouldn't the runtime classpath include all that stuff?

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 recall I explicitly added the getOutput() to this quite a while ago. I don't recall why that is needed. The compileclasspath is required as we have a lot of compileOnly dependencies declared and we need those to run forbiddenapis check successfully.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

@breskeby breskeby added the buildkite-opt-in Opts your PR into Buildkite instead of Jenkins label Nov 1, 2023
@breskeby breskeby merged commit 4fa18ff into elastic:main Nov 1, 2023
@breskeby breskeby deleted the improve-forbidden-apis-checks branch November 1, 2023 15:24
@breskeby
Copy link
Contributor Author

breskeby commented Nov 6, 2023

This work was related to #57918

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildkite-opt-in Opts your PR into Buildkite instead of Jenkins :Delivery/Build Build or test infrastructure >enhancement Team:Delivery Meta label for Delivery team v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants