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

lpac: lpac-libmbim-wrapper doesn't work #25035

Open
bam80 opened this issue Sep 25, 2024 · 6 comments · May be fixed by #25036 or #25046
Open

lpac: lpac-libmbim-wrapper doesn't work #25035

bam80 opened this issue Sep 25, 2024 · 6 comments · May be fixed by #25036 or #25046

Comments

@bam80
Copy link
Contributor

bam80 commented Sep 25, 2024

Maintainer: @blocktrron

Description:
lpac.sh wrapper is currently installed by the name of the binary in /usr/bin/lpac:

$(INSTALL_BIN) ./files/lpac.sh $(1)/usr/bin/lpac

This creates problem as the wrapper overrides env variables which can be set by other upper wrappers.

For now, https://github.com/stich86/lpac-libmbim-wrapper/ doesn't work.

lpac.sh must not mask real binary - instead, it should be installed separately.

bam80 added a commit to bam80/packages that referenced this issue Sep 25, 2024
fixes openwrt#25035 

Signed-off-by: Andrey Butirsky <butirsky@gmail.com>
@bam80 bam80 linked a pull request Sep 25, 2024 that will close this issue
@blocktrron
Copy link
Member

Wrapping the command is to enable usage with uci.

Looking at the project you are referring to, please make https://github.com/stich86/lpac-libmbim-wrapper/blob/main/lpac-mbim#L119C5-L119C8 compatible to supply a --lpac-path argument.

@bam80
Copy link
Contributor Author

bam80 commented Sep 26, 2024

Wrapping the command is to enable usage with uci.

Still, the path is not required to be the same as the binary, right? Use whatever path for your wrapper, but do not touch the binary path, it's not yours.

I think the upstream project behavior is correct, and shouldn't need changes.
Any wrapper should be able to rely on standard path for the binary, adopting it just for the purpose of your hack shouldn't be needed.

If I'm missing something, please let me know.

@blocktrron
Copy link
Member

Use whatever path for your wrapper, but do not touch the binary path, it's not yours.

Where should i put it if not in PATH? Defaulting to configuration is sane behavior for OpenWrt, as this is where the configuration is read from.

If what you require is to pass the command-line arguments to the wrapper and override the envrionment it modifies, you can check if the variables are pre-set in the environment and skip the assignment from UCI in the wrapper.

@bam80
Copy link
Contributor Author

bam80 commented Sep 26, 2024

Where should i put it if not in PATH?

the PATH is OK, just name it differently, I mean. Nobody will complain.

you can check if the variables are pre-set in the environment and skip the assignment from UCI in the wrapper.

I do not see why would we want to undertake such complexity if the fix is as simple as rename your wrapper.
Still need to see what others say. @BKPepe

Anyway, if you want to go that route, would you like to make the fix yourself?
Just make sure it's reviewed positively before merge, please.
Thank you.

@robimarko
Copy link
Contributor

robimarko commented Sep 27, 2024

I quite like the wrapper as otherwise you gotta do it all manually which is quite annoying and beats the purpose of having a UCI configuration.

bam80 added a commit to bam80/packages that referenced this issue Sep 27, 2024
Some env variables sensitive for proper lpac functioning
can be set by upper lpac wrappers such as lpac-libmbim-wrapper.
If that's the case, we shouldn't override them,
otherwise the wrapper won't work.

Also, we shouldn't interpret values of the debug env
variables. Instead, we just pass them as is if the
corresponding uci optioins exist.

fixes openwrt#25035

Signed-off-by: GitHub <noreply@github.com>
@bam80 bam80 linked a pull request Sep 27, 2024 that will close this issue
@bam80
Copy link
Contributor Author

bam80 commented Sep 27, 2024

If what you require is to pass the command-line arguments to the wrapper and override the envrionment it modifies, you can check if the variables are pre-set in the environment and skip the assignment from UCI in the wrapper.

Please see #25046

bam80 added a commit to bam80/packages that referenced this issue Sep 27, 2024
Some env variables sensitive for proper lpac functioning
can be set by upper lpac wrappers such as lpac-libmbim-wrapper.
If that's the case, we shouldn't override them,
otherwise the wrapper won't work.

Also, we shouldn't interpret values of the debug env
variables. Instead, we just pass them as is if the
corresponding uci optioins exist.

fixes openwrt#25035

Signed-off-by: Andrey Butirsky <butirsky@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants