Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix AutoLogin printf and > issues #662

Merged
merged 3 commits into from
Sep 28, 2024

Conversation

adamperkowski
Copy link
Collaborator

@adamperkowski adamperkowski commented Sep 23, 2024

Type of Change

  • Bug fix
  • Refactoring
  • Hotfix

Description

A LOT of bugs here. Someone (@guruswarupa) didn't test this.

Issues / other PRs related

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my own code.
  • My changes generate no errors/warnings/merge conflicts.

@adamperkowski adamperkowski marked this pull request as ready for review September 23, 2024 22:31
core/tabs/utils/auto-login.sh Outdated Show resolved Hide resolved
core/tabs/utils/auto-login.sh Show resolved Hide resolved
@nnyyxxxx
Copy link
Contributor

@adamperkowski you'll have to go through and find the correct paths for each one, some of these are wrong

@adamperkowski
Copy link
Collaborator Author

adamperkowski commented Sep 23, 2024

@nnyyxxxx Who did this? I'd like to have a friendly talk with them...

@adamperkowski adamperkowski marked this pull request as draft September 23, 2024 22:37
@nnyyxxxx
Copy link
Contributor

@nnyyxxxx Who did this? I'd like to have a friendly talk with them...

guru

@adamperkowski
Copy link
Collaborator Author

@guruswarupa ...

@adamperkowski adamperkowski marked this pull request as ready for review September 23, 2024 22:52
@adamperkowski
Copy link
Collaborator Author

@adamperkowski adamperkowski changed the title Fix AutoLogin printf & > issues Fix AutoLogin printf and > issues Sep 23, 2024
core/tabs/utils/auto-login.sh Outdated Show resolved Hide resolved
@cartercanedy
Copy link
Contributor

cartercanedy commented Sep 23, 2024

I was wrong

The bsd man pages are so much better than the ones for gnutils

@nnyyxxxx
Copy link
Contributor

I was wrong

The bsd man pages are so much better than the ones for gnutils

wrong

@nnyyxxxx
Copy link
Contributor

thats only for bsd

@cartercanedy
Copy link
Contributor

thats only for bsd

?

Which form are you talking about

@nnyyxxxx
Copy link
Contributor

thats only for bsd

?

Which form are you talking about

the empty arg u mentioned in your edited message, only works for linux, bsd on the otherhand it would not work on

@cartercanedy
Copy link
Contributor

cartercanedy commented Sep 23, 2024

thats only for bsd

?

Which form are you talking about

the empty arg u mentioned in your edited message, only works for linux, bsd on the otherhand it would not work on

sed -i'.old' 's/foo/bar/' test.txt produces a file named foo.txt.old

sed -i'' 's/foo/bar/' test.txt does not

@nnyyxxxx
Copy link
Contributor

sed -i'.old' 's/foo/bar/' test.txt

that will work on bsd, but if u take the argument away it wont

@nnyyxxxx
Copy link
Contributor

if there is no argument on linux it will work though, so you were only half right.

@cartercanedy
Copy link
Contributor

sed -i'.old' 's/foo/bar/' test.txt

that will work on bsd, but if u take the argument away it wont

Correct, that's why I suggested the change

@cartercanedy
Copy link
Contributor

cartercanedy commented Sep 24, 2024

I wasn't saying that I was wrong to suggest the change, just that the empty arg needed to be concatenated to the flag @nnyyxxxx

@nnyyxxxx
Copy link
Contributor

I wasn't saying that I was wrong to suggest the change, just that the empty arg needed to be concatenated to the flag @nnyyxxxx

i understand i was only saying that you were partially wrong

@cartercanedy
Copy link
Contributor

Ok, glad we agree...
Right?

Copy link
Contributor

@cartercanedy cartercanedy left a comment

Choose a reason for hiding this comment

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

:)

Co-authored-by: cartercanedy <cartercanedy@users.noreply.github.com>
Co-authored-by: nnyyxxxx <nnyyxxxx@users.noreply.github.com>
@adamperkowski
Copy link
Collaborator Author

Co-authors for you :)

@guruswarupa
Copy link
Contributor

@guruswarupa ...

It seemed to work when those lines which you changed were echo. Later while changing it to printf, it broke. I did test on my system before adding it.

@cartercanedy
Copy link
Contributor

cartercanedy commented Sep 24, 2024

It seemed to work when those lines which you changed were echo. Later while changing it to printf, it broke. I did test on my system before adding it.

On its face that's incorrect. The single string argument being passed to sudo will fail because it will assume that it's looking for an executable named "echo '...' > ...", which fails regardless of whether you substitute echo for printf. You'd either have to run sudo bash -c "..." or do what @adamperkowski did and utilize sudo tee to get the permissions right

@nnyyxxxx
Copy link
Contributor

It seemed to work when those lines which you changed were echo. Later while changing it to printf, it broke. I did test on my system before adding it.

On its face that's incorrect. The single string argument being passed to sudo will fail because it will assume that it's looking for an executable named "echo '...' > ...", which fails regardless of whether you substitute echo for printf. You'd either have to run sudo bash -c "..." or do what @adamperkowski did and utilize sudo tee to get the permissions right

i'd recommend against using sub shells

@ChrisTitusTech ChrisTitusTech merged commit 2d1e45a into ChrisTitusTech:main Sep 28, 2024
2 checks passed
@adamperkowski adamperkowski deleted the autologin_tee_fix branch September 28, 2024 18:56
@ChrisTitusTech ChrisTitusTech added the bug Something isn't working label Sep 28, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AutoLogin not working
5 participants