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

Ensure we register a cycle if we've got scala sources but pipelining is disabled #1511

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

ephemerist
Copy link
Contributor

No description provided.

@ephemerist
Copy link
Contributor Author

THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE Organization Contribution License Agreement v2.0, dated July 13, 2012.THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

@ephemerist ephemerist force-pushed the feature/cycles-registration branch from 6dcfc62 to ab5dab3 Compare December 4, 2024 14:37
@ephemerist
Copy link
Contributor Author

Getting a 500 error trying to sign the Scala CLA

@lrytz
Copy link
Contributor

lrytz commented Dec 4, 2024

Getting a 500 error trying to sign the Scala CLA

I reported it

@ephemerist ephemerist force-pushed the feature/cycles-registration branch from ab5dab3 to 7fec355 Compare December 4, 2024 17:05
@ephemerist
Copy link
Contributor Author

Getting a 500 error trying to sign the Scala CLA

I reported it

Thanks. Scala CLA now complete.

@eed3si9n
Copy link
Member

eed3si9n commented Dec 5, 2024

Thanks for the contribution. While this is fresh to you, could you retroactively create an issue describing the bug you're fixing here?

@ephemerist
Copy link
Contributor Author

Thanks for the contribution. While this is fresh to you, could you retroactively create an issue describing the bug you're fixing here?

Added as #1512 - let me know if you'd like any more detail.

@@ -70,14 +70,15 @@ object Incremental {
* Merge latest analysis as of pickling into pruned previous analysis, compute invalidations
* and decide whether we need another cycle.
*/
def mergeAndInvalidate(partialAnalysis: Analysis, completingCycle: Boolean): CompileCycleResult
def mergeAndInvalidate(partialAnalysis: Analysis, registerCycle: Boolean): CompileCycleResult
Copy link
Member

Choose a reason for hiding this comment

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

nit: consider renaming this to something like shouldRegisterCycle since def registerCycle exists, which this flag is encouraging the implementer to later call.


/**
* Merge latest analysis as of analyzer into pruned previous analysis and inform file manager.
*/
def completeCycle(
prev: Option[CompileCycleResult],
partialAnalysis: Analysis
partialAnalysis: Analysis,
registerCycle: Boolean
Copy link
Member

Choose a reason for hiding this comment

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

nit: same here.

incHandlerOpt foreach { incHandler =>
if (earlyOutput.isDefined && incHandler.isFullCompilation) {
val a = getAnalysis
val CompileCycleResult(continue, invalidations, merged) =
incHandler.mergeAndInvalidate(a, false)
incHandler.mergeAndInvalidate(a, true)
Copy link
Member

Choose a reason for hiding this comment

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

to avoid boolean blindness, could we add parameterName = true here plz?

@ephemerist ephemerist force-pushed the feature/cycles-registration branch 2 times, most recently from 065c3a8 to 6c520f5 Compare December 5, 2024 07:06
@ephemerist ephemerist force-pushed the feature/cycles-registration branch from 6c520f5 to f032a9a Compare December 5, 2024 07:28
@ephemerist
Copy link
Contributor Author

ephemerist commented Dec 5, 2024

@eed3si9n thanks for the review - I've addressed all your comments. Not sure why Scala CLA is still failing - I clicked the link and it says {"user":"ephemerist","signed":true,"version":"1.0","currentVersion":"1.0"}

@lrytz
Copy link
Contributor

lrytz commented Dec 5, 2024

Not sure why Scala CLA is still failing

Seems the checker doesn't follow redirects. #1513

Copy link
Member

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Thanks!

@eed3si9n eed3si9n merged commit 5ca7ed7 into sbt:1.10.x Dec 6, 2024
9 checks passed
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.

3 participants