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

chore: Temporarily disable the license report generation to speed up build #704

Merged
merged 14 commits into from
Jan 23, 2024

Conversation

Roiocam
Copy link
Member

@Roiocam Roiocam commented Oct 11, 2023

Motivation

Refs: original from #326 and #332, then #1019

This PR improves the speed of paradox build by disabling sbt-license-report on specific build.

PTAL, @mdedetrich @He-Pin

Copy link
Member Author

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

Approach that skip license-report

@@ -35,7 +35,7 @@ jobs:
uses: coursier/setup-action@v1.3.3

- name: Create the Pekko site
run: sbt -Dpekko.genjavadoc.enabled=true "Javaunidoc/doc; Compile/unidoc; docs/paradox"
run: sbt -Dpekko.genjavadoc.enabled=true -Dpekko.genlicensereport.enabled=false "Javaunidoc/doc; Compile/unidoc; docs/paradox"
Copy link
Member Author

Choose a reason for hiding this comment

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

skip the license-report page generation in link-validator step.

Copy link
Member

Choose a reason for hiding this comment

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

As this is a ci, can we keep it? or we add a dedicated task for genlicensereport

project/ProjectIndexGenerator.scala Outdated Show resolved Hide resolved
scripts/link-validator.conf Outdated Show resolved Hide resolved
@Roiocam Roiocam marked this pull request as draft October 12, 2023 08:32
@Roiocam Roiocam marked this pull request as ready for review October 12, 2023 09:38
@Roiocam
Copy link
Member Author

Roiocam commented Oct 12, 2023

Looks like last CI has successfully passed. And since the PR was opened, every Link Validator job in each commit has been successfully passed. It seems that this approach can address the issues caused by the license report.

One more commit for documentation clarify.

@pjfanning could you trigger the CI again? Thanks.

@mdedetrich
Copy link
Contributor

mdedetrich commented Oct 13, 2023

I had another way of solving this issue which involved adding an ignore-files config to my forked https://github.com/mdedetrich/site-link-validator (the fork is also there so we don't rely on akka). Such a configuration would provide a far simpler solution as you would just do something like

ignore-files = [
  "link-validator.html"
]

in scripts/link-validator.conf

Which makes this really annoying because a lot of time was spent on this :/

@Roiocam
Copy link
Member Author

Roiocam commented Oct 13, 2023

I had another way of solving this issue which involved adding an ignore-files config to my forked https://github.com/mdedetrich/site-link-validator (the fork is also there so we don't rely on akka).

I have to say that this approach also crossed my mind initially. However, considering the additional maintenance cost of another repo, I have decided to abandon this solution for this issue and consider a simpler approach.

This PR approach more than ignores license report files and could skip the task of license report generation, it can make PR checks run faster.

In addition, maintaining a new repo could provide ongoing solutions for future issues that may arise. However, the biggest obstacle for me is the sustainability of maintaining an additional repo. Throughout my career, I have encountered situations where repositories were no longer updated due to a lack of maintenance personnel, such as Apache Common DButils. (It's just my stubborn mindset.)

Anyway, I am willing to accept whichever solution is ultimately accepted because both of them address the problem.

Best regards.

@mdedetrich
Copy link
Contributor

So my biggest apprehension is the complexity of the build which this PR does increase, I have an overarching goal in my head to reduce to try and gradually simplify the sbt build (I haven't really formalized this yet) as I see that sbt's build complexity is creating a barrier of entry to those trying to contribute.

The reduction of the build time I don't see as a critical priority in this specific case, also do note that there will be movement in the future to try and procure our own hardware.

@Roiocam
Copy link
Member Author

Roiocam commented Oct 14, 2023

@mdedetrich Another viewpoint argues that those repo(link-validator) is not significant in scale and does not require regular updates. Therefore, the lack of maintenance is not a significant issue to consider.

Scala is not my primary language, but I still can make the commit like this PR. I acknowledge that the sbt build can be a bit complex, but also offers a lot of possibilities. Considering that sbt builds are not something that needs frequent changes, I believe it won't hinder those who are trying to contribute.

Perhaps we can merge this PR first to avoid those annoying check failure. Once the PR for the other repo is merged and released, we can remove the redundant build task from this PR (This PR also includes a fix part for the broken links). Unless the repo of https://github.com/mdedetrich/site-link-validator was proceeding well.

@Roiocam
Copy link
Member Author

Roiocam commented Oct 25, 2023

@mdedetrich any feedback of this issue will be great.

@Roiocam Roiocam force-pushed the flaky-link-validator branch 3 times, most recently from 4681235 to 0c964fe Compare December 21, 2023 11:14
@Roiocam Roiocam changed the title fix: flaky link validator test feat: improve the speed of paradox build Jan 22, 2024
"links.md")

val markdownFiles = if (CliOptions.generateLicenseReportEnabled.get) {
markdownFilesBeforeLicense ++ Seq("license-report.md") ++ markdownFilesAfterLicense
Copy link
Member

Choose a reason for hiding this comment

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

I think we can add a paradoxFast and paradoxFastBrowse task which turn this off?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have tried those approach:

  • (Compile / paradox / javaOptions += "-Dpekko.genlicensereport.enabled=false")
  • (javaOptions += "-Dpekko.genlicensereport.enabled=false")
  • Compile / paradoxMarkdownToHtml / sourceGenerators := { (Compile / paradoxMarkdownToHtml / sourceGenerators).value.filterNot(_ == Paradox.licenseReportGeneratorTask.taskValue) }
  • Compile / paradoxMarkdownToHtml / sourceGenerators := { Seq.empty }

None of them can be filtered in the license report generated, will keep find a new approach tomorrow.

@mdedetrich
Copy link
Contributor

So I am not sure how this PR relates to #1019 specifically but as explained here the majority of time is taken up in sbt-license-report's updateLicenses task.

I would divert effort into solving that fundamental issue

@pjfanning
Copy link
Contributor

@Roiocam could I ask you to get an iCLA? https://www.apache.org/licenses/contributor-agreements.html

It makes the team's life easier if regular contributors have iCLAs

@Roiocam
Copy link
Member Author

Roiocam commented Jan 22, 2024

@Roiocam could I ask you to get an iCLA? https://www.apache.org/licenses/contributor-agreements.html

It makes the team's life easier if regular contributors have iCLAs

I was sent the ICLA submission on 2023/9/23, is any thing I have missed?

@pjfanning
Copy link
Contributor

@Roiocam thanks - I found your iCLA.

@mdedetrich
Copy link
Contributor

So I am fine with this being a temporary fix but when sbt/sbt-license-report#87 is resolved I would suggest to revert this commit because there isn't any reason to have this feature disable i.e. its currently a workaround for that bug.

This is because when sbt/sbt-license-report#87 is resolved, the updateLicenses task should be near instant because it will actually re-use all of the dependency resolution that was already done when sbt is loaded.

@Roiocam Roiocam changed the title feat: improve the speed of paradox build chore: Temporarily disable the license report generation to speed up build Jan 23, 2024
@Roiocam
Copy link
Member Author

Roiocam commented Jan 23, 2024

So I am fine with this being a temporary fix but when sbt/sbt-license-report#87 is resolved I would suggest to revert this commit because there isn't any reason to have this feature disable i.e. its currently a workaround for that bug.

Yes, that's why we still need another ISSUE #1019 to associate the problem.

This is because when sbt/sbt-license-report#87 is resolved, the updateLicenses task should be near instant because it will actually re-use all of the dependency resolution that was already done when sbt is loaded.

No doubt it, this PR is just sugar for contributors now.

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Okay I am going to approve it, would be good if @pjfanning and @He-Pin also looked at it. @Roiocam Can you make an standard issue to revert this PR once sbt/sbt-license-report#87 is solved so we don't forget.

Thanks for the work!

@Roiocam
Copy link
Member Author

Roiocam commented Jan 23, 2024

Okay I am going to approve it, would be good if @pjfanning and @He-Pin also looked at it. @Roiocam Can you make an standard issue to revert this PR once sbt/sbt-license-report#87 is solved so we don't forget.

Yes, I will stay updated on the progress.

Copy link
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

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

Thanks

@He-Pin
Copy link
Member

He-Pin commented Jan 23, 2024

@pjfanning Would you like to take a look at this, I think with this , all our contributors can have a faster local development exprirence.

@He-Pin He-Pin requested a review from pjfanning January 23, 2024 11:11
@He-Pin He-Pin merged commit 108f5d1 into apache:main Jan 23, 2024
18 checks passed
@Roiocam Roiocam mentioned this pull request Jan 26, 2024
32 tasks
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.

4 participants