Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@matanlurey
Copy link
Contributor

... as part of buildmoot.

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@yaakovschectman
Copy link
Contributor

From Android triage: FYI, the last time resources were moved, Java linting failed silently.

@matanlurey
Copy link
Contributor Author

From Android triage: FYI, the last time resources were moved, Java linting failed silently.

Sorry what does that mean? Don't land this? What linting?

@yaakovschectman
Copy link
Contributor

@reidbaker Any thoughts? Does our previous experience with migrating resources amount to anything more than an FYI here, or impact how this PR should move forward in any way?

@reidbaker
Copy link
Contributor

It means be on the lookout for weird behavior related to java. Tests passing is not a good indication of stability when it comes to moving the locations of resources.

@matanlurey
Copy link
Contributor Author

It means be on the lookout for weird behavior related to java. Tests passing is not a good indication of stability when it comes to moving the locations of resources.

I don't think that kind of comment is very helpful, I can't possibly be on the lookout for behavior that isn't triggered on CI.

Maybe what you're saying is "we would like to have more assurance Java linting is working/enforced on CI before landing this change", which is a reasonable request. By Java linting, I assume you mean android_lint?

If so I could add a CI check that intentionally makes a change that should be blocked, for example. Wdut?

/cc @gaaclarke if you have any thoughts about the above.

@gaaclarke
Copy link
Member

I'm unfamiliar with the linting problems of the past but I agree that we shouldn't accommodate any behavior that isn't tested on CI. I think intentionally introducing a linting exception that the linter catches it as part of the CI checks makes sense if this is something we want to make sure doesn't break in the future.

@matanlurey
Copy link
Contributor Author

I will wait to land this Monday, so there isn't engine breaks of LUCI cache weirdness over the weekend.

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 3, 2024
@auto-submit
Copy link
Contributor

auto-submit bot commented Jul 3, 2024

auto label is removed for flutter/engine/53590, due to - The status or check suite Linux linux_android_debug_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@chinmaygarde
Copy link
Member

The failures do look related. Perhaps you missed a spot where the paths need a fixup?

@matanlurey
Copy link
Contributor Author

The failures do look related. Perhaps you missed a spot where the paths need a fixup?

Updated!

auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jul 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants