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

UP-TO-DATE checking for npm based step config #2192

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

simschla
Copy link
Contributor

@simschla simschla commented Jul 3, 2024

[DRAFT!] This PR introduces correct UP-TO-DATE checking for the npm based steps.

As discussed in #2140 I've tried both ways: Using (1) the current "serialization-as-equality-hack" or (2) implementing FormatterStep directly. After playing around, I think approach (2) leads to a simpler and clearer implementation.

So in this PR, I'd like to discuss this second approach based on the changed implementation of the EslintFormatterStep before I go all the way and change also TsFmt and Prettier.

@simschla simschla requested a review from nedtwigg July 3, 2024 05:45
@Retention(RetentionPolicy.SOURCE)
@Documented
@Target(ElementType.FIELD)
@interface IrrelevantForEquality {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

will probably drop this again as it turned out that I don't need it.

* 1. Equals and hashcode should not include absolute paths and such - optimize for buildcache keys
* 2. Serialized/deserialized state can include absolute paths and such and should recreate a valid/runnable state
*/
class RoundtrippableFile implements Serializable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is inspired by FileSignature - except that it does not use the serializable-for-equality-hack.
Maybe we could combine these two classes?

// => equals/hashcode should not include absolute paths and such
// => serialized/deserialized state can include absolute paths and such and should recreate a valid/runnable state

private final Map<NpmConfigElement, Serializable> configElements = new HashMap<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This map contains all state for the FormatterStep that should be respected for equals/hashCode. Everything else is implemented as derived.

// override serialize output
private void writeObject(ObjectOutputStream out) throws IOException {
// TODO (simschla, 27.06.2024): Implement custom serialization if needed
// TODO (simschla, 03.07.2024): Should we stop the server here if it is running?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we?

@nedtwigg
Copy link
Member

nedtwigg commented Jul 3, 2024

(1) the current "serialization-as-equality-hack" or (2) implementing FormatterStep directly.

Hi @simschla, unfortunately I've just learned that maybe (1) isn't going to work with build-cache. Details at

@simschla
Copy link
Contributor Author

(1) the current "serialization-as-equality-hack" or (2) implementing FormatterStep directly.

Hi @simschla, unfortunately I've just learned that maybe (1) isn't going to work with build-cache. Details at

So it is a good thing I went with (2) 👍

Do you see any issues with the general concept or should I go ahead and transform the other two steps also?

@nedtwigg
Copy link
Member

Sorry, a better description of the problem:

  • implementing FormatterStep directly -> definitely can't work with shared build cache and configuration cache at the same time (without a change in Gradle)
  • serialization-as-equality-hack -> possible to work with shared build cache and configuration cache at the same time using a hack, but the whole point of this whole mess was to get rid of hacks

For now I think best to wait and see if Gradle has any thoughts on the core problem.

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.

2 participants