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

Fix for Issue 141, check the executing user not the logged in user for root and add missing sudo calls #142

Closed
wants to merge 1 commit into from

Conversation

mitant
Copy link

@mitant mitant commented Jul 17, 2023

No description provided.

Copy link
Member

@mryel00 mryel00 left a comment

Choose a reason for hiding this comment

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

This PR shouldn't fix #141 according to the things you described inside the issue, only maybe the sudo -u pi make install variant and will break other things in return, like kiauh.
I might look into your issue if I get some spare time, but atm this PR won't be approved and will be closed as it will just break things, and changes the current way of installing crowsnest.
Your new make flag might be a more suited option, but I will have to test and reproduce the issue myself first.

tools/libs/core.sh Show resolved Hide resolved
tools/install.sh Show resolved Hide resolved
@mryel00 mryel00 closed this Jul 31, 2023
@mitant
Copy link
Author

mitant commented Jul 31, 2023

It doesn't seem like you've actually reproduced the error and tested my PR whereas I cannot build with custompios via docker with crowsnest active.

@mryel00
Copy link
Member

mryel00 commented Jul 31, 2023

but I will have to test and reproduce the issue myself first.

I already wrote that I didn't try to reproduce your issue. My feedback wasn't about your issue, it was about crowsnest and how it get's installed by default, and that will break with your changes

@KwadFan
Copy link
Member

KwadFan commented Aug 1, 2023

As @mryel00 already answered, I'd took a look and he is absolutly right to refuse this PR.
To be honest, you should take care of the whole process, before taking action an breaking things besides things that work for you, so I appriciate your try to fix this particular issue but in that form, I will never merge it anyways.

Regards Kwad

@mitant
Copy link
Author

mitant commented Aug 1, 2023

As @mryel00 already answered, I'd took a look and he is absolutly right to refuse this PR. To be honest, you should take care of the whole process, before taking action an breaking things besides things that work for you, so I appriciate your try to fix this particular issue but in that form, I will never merge it anyways.

Regards Kwad

I'm not sure if your comment is after reading how @mryel00 and I discussed how to move this forward or if you are not interested in improving this PR. Please clarify so I know if I am going to be productive in contributing to this project or if you're just going to reject the new PR even though you have not tested or tried anything. Your other comment about what SUDO_USER is when compiling via docker/logged in as root is contradicting your own team member's experience.

@mryel00
Copy link
Member

mryel00 commented Aug 1, 2023

Your other comment about what SUDO_USER is when compiling via docker/logged in as root is contradicting your own team member's experience.

About that, I guess you misunderstood the message of kwad inside the conversation. He meant if your SUDO_USER is root, in your current building setup, there is most likely a misconfiguration inside your base module of custompios. I don't know much about custompios, that's why I mainly focused on the installation problems occuring outside of it, with your changes.

kwad and me are both no native english speakers, so there might be some misunderstandings sometimes.

Also he wrote inside the conversation that he wants your exact setup, so the base image, every module and so on that you are using, that we can see if you just made a mistake maybe, and that we can reproduce it.

@mitant
Copy link
Author

mitant commented Aug 1, 2023

here's the pi image i'm building https://github.com/mitant/RatOS

Note: the HEAD does not have any of my changes to use the new PR stuff, so if you build in docker using custompios instructions, you should see the failure I was seeing. I have some uncommited changes I have not pushed, but the changes are to point to my fork and to call sudo -u "${BASE_USER}" make install in the crowsnest start_chroot_script. this will be changed to something like make install-docker-custompios to use the new target, if that is going to be acceptable.

I'm using the dockerfile as suggested by guysoft/custompios with some small name changes: https://github.com/guysoft/CustomPiOS/wiki/Building-with-Docker
https://github.com/guysoft/CustomPiOS/wiki/Building#quickstart---i-want-to-build-a-new-distro-using-docker

`version: '3.6'

services:
custompios:
image: guysoft/custompios:devel
container_name: RatOS-builder
tty: true
restart: always
privileged: true
volumes:
- ./src:/distro
devices:
- /dev/loop-control
`

after I start the container I run:
docker exec -it RatOS-builder build

this is the error i see:

Processing triggers for libgdk-pixbuf-2.0-0:arm64 (2.42.2+dfsg-1+deb11u1) ...
�[92mLaunch crowsnest install routine ...

	Please do NOT run this script as root!

	Login in as regular user and run with '�[32msudo make install�[0m'


make: *** [Makefile:17: install] Error 1
++++ echo_red 'build failed, unmounting image...'
++++ echo -e -n '\e[91m'

@mitant
Copy link
Author

mitant commented Aug 1, 2023

forgot to mention, using this base image
2023-05-03-raspios-bullseye-arm64-lite.img

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.

4 participants