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 up-to-date checks for precommit related tasks #57203

Conversation

breskeby
Copy link
Contributor

  • Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
  • Use marker output folder for dependencies license task to make task incremental build compliant

@breskeby breskeby self-assigned this May 27, 2020
@breskeby breskeby added :Delivery/Build Build or test infrastructure Team:Core/Infra Meta label for core/infra team v7.7.1 v7.8.1 v7.9.0 v8.0.0 labels May 27, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Build)

@breskeby breskeby requested review from mark-vieira and rjernst May 27, 2020 12:44
Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

The changes LGTM.

I was wondering whether it's worth defining a wrapper of some kind, so that we don't have to keep repeating the "Explicitly using an Action interface" comments. Something like:

jarTask.doFirst(TaskWrapper.of(task -> { ... }))

where the implementation is something like:

public class TaskWrapper {
    /**
     * Explicitly use an Action interface as Java lambdas
     * are not supported by Gradle up-to-date checks
     */
    public static Action<Task> of(Consumer<Task> fn) {
        return new Action<Task>() {
            @Override
            public void execute(Task task) {
                fn.call(task);
            }
        }
    }
}

@breskeby
Copy link
Contributor Author

breskeby commented May 27, 2020

The changes LGTM.

I was wondering whether it's worth defining a wrapper of some kind, so that we don't have to keep repeating the "Explicitly using an Action interface" comments. Something like:

jarTask.doFirst(TaskWrapper.of(task -> { ... }))

where the implementation is something like:

public class TaskWrapper {
    /**
     * Explicitly use an Action interface as Java lambdas
     * are not supported by Gradle up-to-date checks
     */
    public static Action<Task> of(Consumer<Task> fn) {
        return new Action<Task>() {
            @Override
            public void execute(Task task) {
                fn.call(task);
            }
        }
    }
}

@pugnascotia Neat idea, but I checked and this wrapper could potentially break the up-to-date check when the implementation of the lambda changes. The problem is that a change in the implementationof a lambda cannot be tracked properly by gradle

breskeby added 2 commits May 27, 2020 17:14
- Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
- Use marker output folder for dependencies license task to make task incremental build compliant
@breskeby breskeby force-pushed the improvement/fix-copy-checkstyle-conf-incrementality branch from 0497c8b to c4c2548 Compare May 27, 2020 15:15
Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Changes LGTM 👍

@pugnascotia
Copy link
Contributor

@breskeby Ah, that's shame. Never mind then. 👍

@breskeby breskeby merged commit c13434d into elastic:master May 28, 2020
@breskeby breskeby deleted the improvement/fix-copy-checkstyle-conf-incrementality branch May 28, 2020 12:42
breskeby added a commit to breskeby/elasticsearch that referenced this pull request May 28, 2020
* Fix up-to-date checks for precommit related tasks

- Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
- Use marker output folder for dependencies license task to make task incremental build compliant

* Tweak formatting
breskeby added a commit to breskeby/elasticsearch that referenced this pull request May 28, 2020
* Fix up-to-date checks for precommit related tasks

- Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
- Use marker output folder for dependencies license task to make task incremental build compliant

* Tweak formatting
breskeby added a commit to breskeby/elasticsearch that referenced this pull request May 28, 2020
* Fix up-to-date checks for precommit related tasks

- Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
- Use marker output folder for dependencies license task to make task incremental build compliant

* Tweak formatting
breskeby added a commit that referenced this pull request May 28, 2020
* Fix up-to-date checks for precommit related tasks

- Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
- Use marker output folder for dependencies license task to make task incremental build compliant

* Tweak formatting
breskeby added a commit that referenced this pull request May 28, 2020
* Fix up-to-date checks for precommit related tasks

- Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
- Use marker output folder for dependencies license task to make task incremental build compliant

* Tweak formatting
breskeby added a commit that referenced this pull request May 28, 2020
* Fix up-to-date checks for precommit related tasks

- Do not use lambdas for doFirst / doLast action declarations as this is not supported by gradle up-to-date check
- Use marker output folder for dependencies license task to make task incremental build compliant

* Tweak formatting
@mark-vieira mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v7.7.1 v7.8.1 v7.9.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants