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

cross-cc.mk: Fix slowness since previous fix #5998

Closed
wants to merge 1 commit into from

Conversation

th0ma7
Copy link
Contributor

@th0ma7 th0ma7 commented Jan 29, 2024

Description

cross-cc.mk: Fix slowness noticed by @mreid-tt since previous fix

Fixes #5995 (comment)

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

@th0ma7 th0ma7 requested a review from mreid-tt January 29, 2024 17:51
@th0ma7
Copy link
Contributor Author

th0ma7 commented Jan 29, 2024

@mreid-tt As soon as I read your question #5995 (comment) I knew where this may be comming from. I've changed the if/else so it a) makes more sense and b) avoid causing any delay at build time.

Initial testing seems to demonstrate it works well, do you mind confirming on your end before I merge?

@mreid-tt
Copy link
Contributor

mreid-tt commented Jan 29, 2024

Initial testing seems to demonstrate it works well, do you mind confirming on your end before I merge?

Given that I only build via GitHub, testing would be a bit difficult. My understanding is that I would need to rebase to the current master and then import your commit to be able to test it. Is this correct?

EDIT: I would recommend giving this PR a more standardised title and description before merging.

@mreid-tt mreid-tt changed the title cross-cc.mk: Fix slowness noticed by @mreid-tt since previous fix cross-cc.mk: Fix slowness since previous fix Jan 29, 2024
@mreid-tt
Copy link
Contributor

@th0ma7, I did the rebase I suggested above and merged in your PR but so far the "Evaluating dependencies" is running for over 10 minutes which is unusual. Will continue to monitor at: https://github.com/SynoCommunity/spksrc/pull/5992/checks

@th0ma7
Copy link
Contributor Author

th0ma7 commented Jan 29, 2024

wow, that took 1h59mins... this is odd. Let me revisit this.

@mreid-tt do you have previous runs we can check on?

EDIT: Looking at https://github.com/SynoCommunity/spksrc/pull/5879/checks which was not rebased yet (not even to prior change) took 3m15secs. So that a baseline I can work with.

@mreid-tt
Copy link
Contributor

mreid-tt commented Jan 29, 2024

@mreid-tt do you have previous runs we can check on?

You can take a look at the previous successful builds which were all a few minutes until 2 days ago: https://github.com/SynoCommunity/spksrc/actions/workflows/build.yml?query=is%3Asuccess+branch%3Afengoffice-update

@th0ma7
Copy link
Contributor Author

th0ma7 commented Jan 31, 2024

@hgy59 and @mreid-tt I've now reverted the change, and also identified why. I'll rework something out that may be able to be used for cmake, rust and others later on.

@mreid-tt mreid-tt removed their request for review January 31, 2024 12:14
@mreid-tt
Copy link
Contributor

@th0ma7, thanks much for this. I have rebased my open PR #5992 and can confirm that the "Evaluate dependencies" step is back to taking the expected time (3m 24s on the last run).

@th0ma7 th0ma7 mentioned this pull request Feb 1, 2024
48 tasks
@th0ma7
Copy link
Contributor Author

th0ma7 commented Feb 1, 2024

Superseeded with #6002

@th0ma7 th0ma7 closed this Feb 1, 2024
@th0ma7 th0ma7 deleted the fix-cross-cc branch February 1, 2024 02:47
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.

2 participants