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

Remove dependency that creates a cycle. #5777

Merged
merged 5 commits into from
Apr 8, 2023

Conversation

TWiStErRob
Copy link
Member

@TWiStErRob TWiStErRob commented Feb 9, 2023

Forked from #5773 (comment)

Since a dependency is removed in this PR, now the warning can trigger as described in the conversation thread on the other PR. This will happen randomly based on parallel ordering. Luckily we got a failure on one of the matrix jobs and Matthew noticed it.

A consistent repro is

gradlew :detekt-generator:generateDocumentation :detekt-formatting:cleanProcessResources :detekt-formatting:processResources --console=verbose

which works on the Remove dependency that creates a cycle. commit. After that it should be fixed.

@github-actions github-actions bot added the build label Feb 9, 2023
@codecov
Copy link

codecov bot commented Feb 9, 2023

Codecov Report

Merging #5777 (497c90d) into main (0ddd244) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #5777   +/-   ##
=========================================
  Coverage     84.59%   84.59%           
  Complexity     3784     3784           
=========================================
  Files           546      546           
  Lines         12943    12943           
  Branches       2273     2273           
=========================================
  Hits          10949    10949           
  Misses          861      861           
  Partials       1133     1133           
Impacted Files Coverage Δ
...urbosch/detekt/generator/collection/RuleVisitor.kt 89.89% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@3flex
Copy link
Member

3flex commented Feb 22, 2023

I think this can be closed now?

@TWiStErRob
Copy link
Member Author

I would still continue this way... the fix in #5792 was not complete, it's just working around the issues.

@3flex
Copy link
Member

3flex commented Mar 2, 2023

I've been thinking about this and agree it should be merged. Was there anything else that you're planning to do for this PR?

@3flex 3flex self-requested a review March 2, 2023 23:15
@TWiStErRob TWiStErRob marked this pull request as ready for review March 4, 2023 09:50
@github-actions
Copy link

github-actions bot commented Mar 4, 2023

Warnings
⚠️ It looks like this PR contains functional changes without a corresponding test.
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

Generated by 🚫 dangerJS against 084c616

@TWiStErRob TWiStErRob added this to the 1.23.0 milestone Mar 4, 2023
@TWiStErRob
Copy link
Member Author

TWiStErRob commented Mar 4, 2023

I think it might be enough to just do this. Can't think of anything else. Originally I only raised the PR to show the issue, but looking at it, it might be production ready 🤠. I fixed a typos and updated with latest to see if it still works.

@3flex
Copy link
Member

3flex commented Mar 4, 2023

I'm good with the PR itself but I thought these errors were dealt with? https://github.com/detekt/detekt/actions/runs/4330440036/jobs/7561663181#step:4:520

Is that new with this change?

@TWiStErRob
Copy link
Member Author

Ah, yes! Lucky flaky build. Need to fix that. Now that I broke the circular dependency, the ordering is reversed (randomly). We can now really fix the implicit dependencies. I'll have a look next week.

@TWiStErRob TWiStErRob marked this pull request as draft March 4, 2023 15:24
@cortinico
Copy link
Member

Do we want this included in 1.23 @TWiStErRob ?

@TWiStErRob
Copy link
Member Author

@cortinico No rush, as far as I understand it's more for correctness and tech debt, I don't think users will see anything of it since the change is in detekt-generator. That said I'll spend a little time on it now, hopefully finishing it.

@cortinico cortinico removed this from the 1.23.0 milestone Apr 6, 2023
@cortinico
Copy link
Member

Got it. Removing the milestone for now. If we merge it in a couple of weeks it will make it into 1.23 👍

@TWiStErRob

This comment was marked as duplicate.

@TWiStErRob TWiStErRob marked this pull request as ready for review April 6, 2023 15:25
@TWiStErRob
Copy link
Member Author

@3flex, I fixed the missing dependency similar to earlier fixes by @jprinet. Please have a look if this makes sense, I don't think there's anything else to do for now. In the future we definitely need to split up the config generation and somehow prevent referencing source-sets between projects:

.flatMap { it.sourceSets.main.get().kotlin.srcDirs }

I'm quite surprised this works with configuration cache. Project isolation all the way! 💪

@3flex 3flex merged commit d8da06d into detekt:main Apr 8, 2023
@cortinico cortinico added this to the 1.23.0 milestone Apr 8, 2023
@TWiStErRob TWiStErRob deleted the generator_cycle branch May 19, 2023 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants