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.

Sign up for GitHub

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

Jump to bottom

Unnecessary dependencies #70

Closed
olafdietsche opened this issue Feb 11, 2023 · 6 comments · May be fixed by #71
Closed

Unnecessary dependencies #70

olafdietsche opened this issue Feb 11, 2023 · 6 comments · May be fixed by #71
Labels
bug Something isn't working needs-triage Needs more investigation to determine the priority

Comments

@olafdietsche
Copy link

olafdietsche commented Feb 11, 2023 •
edited

What did you expect to happen?

There should be no dependency to bc, because it isn't used anywhere in the scripts.

What actually happened?

https://github.com/phenax/bsp-layout/blob/master/src/utils/common.sh aborts, if bc isn't installed.

Describe your attempts to resolve the issue

Nothing done so far.

Steps to reproduce

  1. Uninstall bc or rename bc, e.g. mv /usr/bin/bc /usr/bin/bc.off
  2. Run bsp-layout
  3. bsp-layout aborts with "[Missing dependency] bsp-layout needs bc installed"
  4. Don't forget to reinstall bc or revert the renaming: mv /usr/bin/bc.off /usr/bin/bc

System Information

I installed the bsp-layout command with make DESTDIR=/tmp/destdir install.

$ uname -osrm
Linux 5.15.0-56-generic x86_64 GNU/Linux
$ /tmp/destdir/usr/local/bin/bsp-layout version
0.0.10-1
@olafdietsche olafdietsche added bug Something isn't working needs-triage Needs more investigation to determine the priority labels Feb 11, 2023
@olafdietsche olafdietsche changed the title Wrong dependencies Unnecessary dependencies Feb 11, 2023
@olafdietsche
Copy link
Author

Still playing with bsp-layout. When doing bsp-layout set tall everything looks ok.
But when I say bsp-layout once tall, it shows an error

/tmp/destdir/usr/local/lib/bsp-layout/layouts/tall.sh: line 53: 0.6 * 1920 : syntax error: invalid arithmetic operator (error token is ".6 * 1920 ")

But this is with bc available, so maybe this issue (#70) is invalid and there is another bug.

@amtoine
Copy link
Collaborator

amtoine commented Feb 12, 2023

hello @olafdietsche 👋

it appears bc is not used anywhere, good catch 👌

Still playing with bsp-layout. When doing bsp-layout set tall everything looks ok. But when I say bsp-layout once tall, it shows an error

/tmp/destdir/usr/local/lib/bsp-layout/layouts/tall.sh: line 53: 0.6 * 1920 : syntax error: invalid arithmetic operator (error token is ".6 * 1920 ")

But this is with bc available, so maybe this issue (#70) is invalid and there is another bug.

i am able to reproduce this, without bc even installed on my machine and with the dependency removed 🤔

could you file another issue about this bug? 😌

@amtoine amtoine mentioned this issue Feb 12, 2023
Open
@olafdietsche
Copy link
Author

According to bash - Shell Arithmetic and various online sources, bash doesn't handle floating point by itself.
So maybe this is the reason for the bc dependency, even though it isn't used (yet).

The other issue is at https://github.com/phenax/bsp-layout/blob/master/src/layouts/tall.sh#L53:

local want=$(( $master_size * $mon_width ))

and occurs, because of $master_size being 0.6. I haven't figured out, why this happens with once only.

@olafdietsche
Copy link
Author

It looks like commit feae088 introduced this.

There is a change replacing bc with $(( ... )). This is ok as long as there's only integer arithmetic.
But since there's floating point involved here, the change isn't valid. So maybe review this commit again for other possible issues.

@olafdietsche
Copy link
Author

So, finally this issue is invalid and is the result of the mentioned commit.
Therefore I close the issue.

@amtoine
Copy link
Collaborator

amtoine commented Feb 13, 2023

So, finally this issue is invalid and is the result of the mentioned commit.
Therefore I close the issue.

if you still have an issue, even though it's not relevant in this very issue and comes from another commit, you could file a new issue 😋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working needs-triage Needs more investigation to determine the priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

remove bc from the dependencies amtoine/bsp-layout
2 participants
@olafdietsche @amtoine