Skip to content

Added nostrip to CONFIG to avoid illegal binary strips on WSL #1071

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

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

sgourdas
Copy link
Collaborator

@sgourdas sgourdas commented Mar 28, 2024

Fixes #1059

Added the nostrip flag to CONFIG in the kiwix-desktop.pro in case of WSL

@sgourdas sgourdas changed the title Added nostrip to CONFIG to avoid exec strips Added nostrip to CONFIG to avoid illegal strips Mar 28, 2024
@kelson42
Copy link
Collaborator

kelson42 commented Mar 28, 2024

I don't think we want to just remove it everywhere. The best solution would be to do this only for WSL. The second best would be to do that via qmake/makefile option and document the case properly.

@kelson42 kelson42 self-requested a review March 30, 2024 10:50
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

@sgourdas Have you found a way to detect WSL properly?

@sgourdas
Copy link
Collaborator Author

@kelson42 Based on this it seems like checking for the existence of /proc/sys/fs/binfmt_misc/WSLInterop is the most reliable method, IMO as well, to check for WSL. I added a conditional check for this in the .pro file.

If this is fine for you also, I can squash the commits so we can merge.

@kelson42
Copy link
Collaborator

@sgourdas Thx, I will give a try and verify if it works fine for Linux at least.

@kelson42
Copy link
Collaborator

@sgourdas This piece of code deserves a small code comment explaining the situation/solution

@sgourdas
Copy link
Collaborator Author

sgourdas commented Apr 1, 2024

@kelson42 is this fine?

Comment on lines 11 to 14
#Determine if the host system is WSL based on the existence of "/proc/sys/fs/binfmt_misc/WSLInterop"
WSL_CHECK = $$system(test -f /proc/sys/fs/binfmt_misc/WSLInterop && echo true || echo false)
equals(WSL_CHECK, "true"): CONFIG += nostrip

Copy link
Member

Choose a reason for hiding this comment

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

This comment is not so useful. We already see that we are checking existence of /proc/sys/fs/binfmt_misc/WSLInterop in the code. With a small variable renaming we can easily give the hint about what we are checking.

If we add comment, it should be why we do this, not what or how we are doing it.

I suggest:

# install script fails to detect if file is a binary or not on wsl and then try to strip all
# installed files, which obviously not work for all files. So don't strip on WSL.
ARE_WE_IN_WSL = $$system(test -f /proc/sys/fs/binfmt_misc/WSLInterop && echo true || echo false)
equals(ARE_WE_IN_WSL, "true"): CONFIG += nostrip

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Totally agree.

Should we make it smaller, with something like:

# Avoid stripping incompatible files, due to false identification as executables, on WSL
DETECT_WSL = $$system(test -f /proc/sys/fs/binfmt_misc/WSLInterop && echo true || echo false)
equals(DETECT_WSL , "true"): CONFIG += nostrip

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM, please do and squash everything in one commit so we can merge.

@kelson42 kelson42 changed the title Added nostrip to CONFIG to avoid illegal strips Added nostrip to CONFIG to avoid illegal binary strips on WSL Apr 6, 2024
@kelson42 kelson42 self-requested a review April 7, 2024 18:24
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

LGTM

@kelson42 kelson42 merged commit 7e2012d into kiwix:main Apr 7, 2024
4 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.

Trying to install kiwi-desktop results in error, due to strip
3 participants