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.

Sign up for GitHub

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

Jump to bottom

Throw error from bloopGenerate task #1973

Merged
merged 1 commit into from
Jan 9, 2023
Merged

Throw error from bloopGenerate task #1973

merged 1 commit into from
Jan 9, 2023

Conversation

G1ng3r
Copy link
Contributor

@G1ng3r G1ng3r commented Dec 29, 2022

Sbt task bloopGenerate should throw occurred errors to indicate failures during build import

@G1ng3r G1ng3r mentioned this pull request Dec 29, 2022
Open
tgodzik
tgodzik requested changes Dec 29, 2022
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Thanks for the great contribution! I have one question and it seems the jobs are failing due to explicit usage of bloopGenerate := None in some places. You should be able to change those occurrences to the new format.

integrations/sbt-bloop/src/main/scala/bloop/integrations/sbt/SbtBloop.scala Outdated
@@ -1139,6 +1135,12 @@ object BloopDefaults {
BloopKeys.bloopGenerate
.all(filter)
.map(_.toSet)
.map(_.map {
case Inc(th) =>
logger.error(s"Couldn't run bloopGenerate. Cause:\n$th")
Copy link
Contributor

Choose a reason for hiding this comment

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

So this should still allow the behaviour introduced in #1663 ? Meaning, fail, but still run bloop generate for all the projects it could.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, nope, that behaviour wont work after. Should we introduce some flag to bloopInstall to allow #1663 behaviour, cause it's look like failing import is preferred action?

Copy link
Contributor Author

@G1ng3r G1ng3r Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

maybe we should introduce some new task which should be run manually for #1663 behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess we can do some changes to keep both behaviours, I'll try

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

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'm not sure about how tasks evaluate, I've read some time ago that there's no order guarantees. If at this point of evaluation all bloopGenerate tasks finished then #1663 is satisfied

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we maybe just flatten the result first? Or do a fold on it? And once we get the final result we can throw the exception.

Copy link
Member

@ckipp01 ckipp01 Dec 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just jumping in here to say that I agree with @tgodzik. I think it's importing to keep the behavior to complete as much as possible and to continue to the end. Once we have the collection of results of all the blooGenerates we should then be able to still determine if there was a single Inc and if so report on it.

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 made some changes, need your approve on that

Copy link
Contributor

Choose a reason for hiding this comment

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

Done! Sorry for the delay!

@G1ng3r G1ng3r force-pushed the feature/generate-task-errors branch 3 times, most recently from b54a7ff to 3df69e8 Compare December 30, 2022 19:37
@ckipp01 ckipp01 self-requested a review January 4, 2023 05:37
@ckipp01 ckipp01 assigned ckipp01 and unassigned ckipp01 Jan 4, 2023
@G1ng3r G1ng3r force-pushed the feature/generate-task-errors branch 2 times, most recently from b125490 to 9d7d839 Compare January 6, 2023 09:23
@G1ng3r
Copy link
Contributor Author

G1ng3r commented Jan 6, 2023

@tgodzik some checks failed with timeouts, is it some problem with my changes? or restart until it goes away?

ckipp01
ckipp01 reviewed Jan 6, 2023
View reviewed changes
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Just a nit on naming. As for the CI, don't worry about it the tests that failed have been very flaky recently.

Thanks a lot for working on this!

@G1ng3r G1ng3r force-pushed the feature/generate-task-errors branch from 9d7d839 to 1d52064 Compare January 7, 2023 14:09
ckipp01
ckipp01 approved these changes Jan 9, 2023
View reviewed changes
Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Great, thanks a lot for this @G1ng3r!

@tgodzik do you have any thoughts or are we ok to merge this?

tgodzik
tgodzik approved these changes Jan 9, 2023
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the great work!

@tgodzik tgodzik merged commit 7f15a3c into scalacenter:main Jan 9, 2023
@kjh618 kjh618 mentioned this pull request Nov 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgodzik tgodzik tgodzik approved these changes

@ckipp01 ckipp01 ckipp01 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@G1ng3r @tgodzik @ckipp01