Skip to content

backend:bug: Fixes the bug that caused breaking of app-linux build #3553

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

BABAR-TAHSEEN55
Copy link

Summary

This PR fixes a bug in which app-linux target did not automatically build the backend for the current (Host) OS which caused binary exec error due to an architecture mismatch. Now the makefile ensures that backend is built for the appropriate architecture before running the backend server

Related Issue

Fixes #3162

Changes

  • Added backend in makefile so as to make sure backend is built for the appropriate architecture before running backend server

Steps to Test

  1. make app-linux
  2. make run-backend

Screenshots (if applicable)

Screencast.from.2025-07-02.19-38-57.webm

Notes for the Reviewer

  • [e.g., This touches the i18n layer, so please check language consistency.]

Fixed a bug where app-linux target did not automatically build the
backend for the current HOST OS which causes binary exec error.
Now the makefile ensures that backend is build before running the
backend server
Copy link

linux-foundation-easycla bot commented Jul 2, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot requested review from ashu8912 and sniok July 2, 2025 17:03
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BABAR-TAHSEEN55
Once this PR has been reviewed and has the lgtm label, please assign sniok for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Welcome @BABAR-TAHSEEN55!

It looks like this is your first PR to kubernetes-sigs/headlamp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/headlamp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 2, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 2, 2025
@BABAR-TAHSEEN55
Copy link
Author

Hey @illume @sniok This pr is ready for review . Can you review it ?

Copy link
Contributor

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

I don't think this is a bug, it's intended so we don't have to compile the backend every time, even if subsequent attempts are faster. The problem with this fix is that it will compile the backend every time.
An improvement would be to add the headlamp-server binary as a dependency of that target, and then a new target to compile the backend if the headlamp-server doesn't exist.

@illume
Copy link
Contributor

illume commented Jul 4, 2025

Running make backend takes 0.5 seconds for me after it's compiled, and 8+ seconds normally. So it doesn't do a full rebuild, and is pretty fast.
(This is on my very slow laptop)

I'm wondering if it's worth it?

pros

  • it solves the make app-linux breaking run-backend problem
  • it makes sure when there's changes to the backend run-backend runs with the latest

Cons:

  • it adds 0.5 seconds extra to run-backend (probably less on good laptops)

@BABAR-TAHSEEN55
Copy link
Author

I don't think this is a bug, it's intended so we don't have to compile the backend every time, even if subsequent attempts are faster. The problem with this fix is that it will compile the backend every time. An improvement would be to add the headlamp-server binary as a dependency of that target, and then a new target to compile the backend if the headlamp-server doesn't exist.

Aah! I think creating a new target might be a good fix
I'll look into it and will raise the pr soon
I might have some doubts
If i stumble upon them , I'll contact you through slack

@BABAR-TAHSEEN55
Copy link
Author

Running make backend takes 0.5 seconds for me after it's compiled, and 8+ seconds normally. So it doesn't do a full rebuild, and is pretty fast. (This is on my very slow laptop)

I'm wondering if it's worth it?

pros

  • it solves the make app-linux breaking run-backend problem
  • it makes sure when there's changes to the backend run-backend runs with the latest

Cons:

  • it adds 0.5 seconds extra to run-backend (probably less on good laptops)

I have i7 13th gen and the building process is Pretty quick
Is building Backend everytime bad?
I'm confused

@illume
Copy link
Contributor

illume commented Jul 4, 2025

An improvement would be to add the headlamp-server binary as a dependency of that target, and then a new target to compile the backend if the headlamp-server doesn't exist.

The problem with having the binary file at backend/headlamp-server target as a dependency is that if it exists, then the bug is not fixed. It will not build, and try to run the arm binary on x86 machines.

I have i7 13th gen and the building process is Pretty quick Is building Backend every time bad? I'm confused

I understand @joaquimrocha not wanting run-backend to take extra time compiling. It is a significant extra time, and personally I'd be happy with that extra 0.5 seconds on run-backend... because it solves these issues:

  • it solves the make app-linux breaking run-backend problem
  • it makes sure when there's changes to the backend run-backend runs with the latest

However, it takes 12 seconds when I'm on my slow windows work laptop to compile when nothing has changed and 68 seconds running make backend from scratch. So definitely that's really bad for windows.


Another potential way to fix it that won't add much extra time to run-backend:
In the run-backend make target you could detect the type of binary if it does not match the host, then it could build headlamp-server with make backend?
(This could just happen in the unix branch inside run-backend, because this issue does not exist on windows.)

To detect if the binary matches the host, it's probably a dozen lines of shell scripting using file headlamp-server and uname -a.

@BABAR-TAHSEEN55 what do you think?

@BABAR-TAHSEEN55
Copy link
Author

An improvement would be to add the headlamp-server binary as a dependency of that target, and then a new target to compile the backend if the headlamp-server doesn't exist.

The problem with having the binary file at backend/headlamp-server target as a dependency is that if it exists, then the bug is not fixed. It will not build, and try to run the arm binary on x86 machines.

I have i7 13th gen and the building process is Pretty quick Is building Backend every time bad? I'm confused

I understand @joaquimrocha not wanting run-backend to take extra time compiling. It is a significant extra time, and personally I'd be happy with that extra 0.5 seconds on run-backend... because it solves these issues:

  • it solves the make app-linux breaking run-backend problem
  • it makes sure when there's changes to the backend run-backend runs with the latest

However, it takes 12 seconds when I'm on my slow windows work laptop to compile when nothing has changed and 68 seconds running make backend from scratch. So definitely that's really bad for windows.

Another potential way to fix it that won't add much extra time to run-backend: In the run-backend make target you could detect the type of binary if it does not match the host, then it could build headlamp-server with make backend? (This could just happen in the unix branch inside run-backend, because this issue does not exist on windows.)

To detect if the binary matches the host, it's probably a dozen lines of shell scripting using file headlamp-server and uname -a.

@BABAR-TAHSEEN55 what do you think?

I think the initial solution is better. If that significant extra time won't be a problem in the future & if its not a problem with compiling backend everytime ,
I think it's better
For detecting in the UNIX branch , that would too add some extra time ? right ? Not as much as before but it'will
I like both your suggestions @illume
What are your thoughts on this ? @joaquimrocha

@joaquimrocha
Copy link
Contributor

Hi. I don't think we need to check for the arch directly after all. We already have the file extension pre-checked. So we can do something like:

.PHONY: backend-check-build
backend-check-build:
	@if [ ! -f backend/headlamp-server${SERVER_EXE_EXT} ] || ! git diff --quiet HEAD -- backend/; then \
		echo "Backend changes detected, rebuilding..."; \
		make backend; \
	else \
		echo "Backend is up to date"; \
	fi

And then we add this target as the dependency to the run-backend:

run-backend: backend-check-build

Should give us the best of both worlds 😉

@BABAR-TAHSEEN55
Copy link
Author

Hi. I don't think we need to check for the arch directly after all. We already have the file extension pre-checked. So we can do something like:

.PHONY: backend-check-build
backend-check-build:
	@if [ ! -f backend/headlamp-server${SERVER_EXE_EXT} ] || ! git diff --quiet HEAD -- backend/; then \
		echo "Backend changes detected, rebuilding..."; \
		make backend; \
	else \
		echo "Backend is up to date"; \
	fi

And then we add this target as the dependency to the run-backend:

run-backend: backend-check-build

Should give us the best of both worlds 😉

Ahhh!!!!
This is clearly better approach!! I didn't think of this before
As you solved and provided the core logic , I feel it wouldn't be right for me to just copy and paste it as my own.
Would you prefer that i continue this pr or should i close it ??😄

@joaquimrocha
Copy link
Contributor

This is clearly better approach!! I didn't think of this before
As you solved and provided the core logic , I feel it wouldn't be right for me to just copy and paste it as my own.
Would you prefer that i continue this pr or should i close it ??😄

Just go for it. No worries.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 9, 2025
@BABAR-TAHSEEN55
Copy link
Author

BABAR-TAHSEEN55 commented Jul 9, 2025

image
@joaquimrocha I am still encountering the same issue .
I think the problem is that ,
the build is of different architecture
Should I add a check that would check what type of binary is the host ? or something similar ?

@BABAR-TAHSEEN55
Copy link
Author

BABAR-TAHSEEN55 commented Jul 9, 2025

I added a check in your previous code and did something like this :

.PHONY: backend-check-build
backend-check-build:
	@if [ ! -f backend/headlamp-server${SERVER_EXE_EXT} ] || ! git diff --quiet HEAD -- backend/; then \
		echo "Backend changes detected, rebuilding..."; \
		make backend; \
	elif ! file backend/headlamp-server${SERVER_EXE_EXT} | grep -q "$(shell uname -m | sed 's/_/-/')"; then \
		echo "Backend architecture mismatch (expected $(shell uname -m)). Rebuilding"; \
		make backend;\
	else \
		echo "Backend is up to date"; \
	fi

It result in this :
Screenshot from 2025-07-09 21-10-52
Screenshot from 2025-07-09 21-16-39

Should I push it ?

@illume
Copy link
Contributor

illume commented Jul 10, 2025

Note, it has to work on windows.

@joaquimrocha
Copy link
Contributor

@vyncent-t Can you please test on windows?

@vyncent-t
Copy link
Contributor

@vyncent-t Can you please test on windows?

sure I will take a look

@vyncent-t
Copy link
Contributor

This does not seem to run on windows

On main I can run make run-backend as normal, but when trying to do the same on this branch this is the error.

'uname' is not recognized as an internal or external command,
operable program or batch file.
process_begin: CreateProcess(NULL, uname -m, ...) failed.
Makefile:100: pipe: Bad file descriptor
! was unexpected at this time.
make: *** [Makefile:100: backend-check-build] Error 255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make app-linux breaks ./backend/headlamp-server
5 participants