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

get-deps: Do not build get-deps all the time #4100

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

rene
Copy link
Contributor

@rene rene commented Jul 24, 2024

Move build commands to the proper target in Makefile so get-deps won't get build every time make -C tools/get-deps is called.

@rene rene requested a review from deitch as a code owner July 24, 2024 12:56
@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

@rene I must have misunderstood the makefile. How does this change it? Won't it still get executed with every call in root of eve to make cache-export or make sbom or any other things?

@rene
Copy link
Contributor Author

rene commented Jul 24, 2024

@rene I must have misunderstood the makefile. How does this change it? Won't it still get executed with every call in root of eve to make cache-export or make sbom or any other things?

Yes @deitch , but at this time get-deps will be already built and no go commands will be called.... the problem was that the build commands were in the target all, so they were getting called every time make -C tools/get-deps was called... now it should run very fast when the get-deps binary is already present....

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

Oh, OK, I see.

Still, do we want to run it every time we call the Makefile? Shouldn't we restrict it in the main Makefile just to targets that need it?

@rene
Copy link
Contributor Author

rene commented Jul 24, 2024

Oh, OK, I see.

Still, do we want to run it every time we call the Makefile? Shouldn't we restrict it in the main Makefile just to targets that need it?

It runs only to generate get-deps and the pkg-deps.mk file, once both are there, it shouldn't call make get-deps again... I can't reproduce it locally....

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

can't reproduce it locally

Yeah but try in CI, where each run is in a new VM, and you will see it being rebuilt over and over, even when not needed.

@rene
Copy link
Contributor Author

rene commented Jul 24, 2024

can't reproduce it locally

Yeah but try in CI, where each run is in a new VM, and you will see it being rebuilt over and over, even when not needed.

ok, I'm checking, but apart from that, this fix is needed anyways... so the PR is still valid...

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

Yeah, fair enough. Let's get it in, then. Can you try and address the Makefile too?

@rene rene force-pushed the fix-get-deps-makefile branch from 28ce918 to 1c7998f Compare July 24, 2024 13:39
@github-actions github-actions bot requested a review from deitch July 24, 2024 13:39
@rene
Copy link
Contributor Author

rene commented Jul 24, 2024

Updates in this PR:

  • Rebase onto master in order to get GH runners updates....

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

The fix in #4102 isn't good. Do not approve or merge this until we get that fixed.

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

#4105 is in, I am testing it in #4103. If it is good, you will be able to rebase this. I will post here.

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

So far, the packages part is working correctly. I cannot report on eve yet, because the test in #4103 is queued up for workers

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

Yes, it all is looking good. @rene rebase on master please and you should be good to go here.

Move build commands to the proper target in Makefile so get-deps won't get
build every time make -C tools/get-deps is called.

Signed-off-by: Renê de Souza Pinto <rene@renesp.com.br>
@rene rene force-pushed the fix-get-deps-makefile branch from 1c7998f to 215d8e9 Compare July 24, 2024 14:48
@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

Tests are running. When they are done, I will kick off eden.

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

Started eden tests.

I also looked at the logs:

@deitch
Copy link
Contributor

deitch commented Jul 24, 2024

Getting some odd eden errors. But at least it is not the pull rate limits! 😁

@eriknordmark eriknordmark merged commit 73ce4bb into lf-edge:master Jul 24, 2024
34 of 35 checks passed
@rene rene deleted the fix-get-deps-makefile branch August 2, 2024 13:56
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