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

emerge-webrsync: Create repo location with $PORTAGE_USERNAME:$PORTAGE_GRPNAME ownership #1390

Closed
wants to merge 2 commits into from

Conversation

duxsco
Copy link
Contributor

@duxsco duxsco commented Oct 22, 2024

@thesamesam
Copy link
Member

The commit message shouldn't be empty, but it also needs discussion of the: a) rationale, and b) trade-offs here at least.

@duxsco
Copy link
Contributor Author

duxsco commented Oct 22, 2024

@thesamesam The commit message isn't empty:

❯ git --no-pager show --no-patch --format=%s
emerge-webrsync: Create repo location with portage:portage ownership

❯ git --no-pager show --no-patch --format=%b
Bug: https://bugs.gentoo.org/707980
Signed-off-by: David Sardari <[email protected]>

❯ git --no-pager show --no-patch --format=%B
emerge-webrsync: Create repo location with portage:portage ownership

Bug: https://bugs.gentoo.org/707980
Signed-off-by: David Sardari <[email protected]>

@thesamesam
Copy link
Member

I (obviously) meant the body needs text.

@floppym
Copy link
Contributor

floppym commented Oct 22, 2024

We really should not hard-code "portage:portage" for this. See https://bugs.gentoo.org/941605#c9.

@duxsco
Copy link
Contributor Author

duxsco commented Oct 22, 2024

We really should not hard-code "portage:portage" for this. See https://bugs.gentoo.org/941605#c9.

Agreed, PORTAGE_USERNAME or similar is the way to go.

@duxsco duxsco marked this pull request as draft October 22, 2024 15:30
@duxsco
Copy link
Contributor Author

duxsco commented Oct 22, 2024

@floppym fyi, I implemented your suggestion.

@duxsco duxsco marked this pull request as ready for review October 22, 2024 15:51
@duxsco duxsco changed the title emerge-webrsync: Create repo location with portage:portage ownership emerge-webrsync: Create repo location with $PORTAGE_USERNAME:$PORTAGE_GRPNAME ownership Oct 22, 2024
Copy link
Contributor

@floppym floppym left a comment

Choose a reason for hiding this comment

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

There's some code in sync_local() that hard codes "portage:portage" as well. Would you mind fixing that up?

bin/emerge-webrsync Outdated Show resolved Hide resolved
bin/emerge-webrsync Outdated Show resolved Hide resolved
@duxsco
Copy link
Contributor Author

duxsco commented Oct 22, 2024

fyi, I created a bug for the missing definition of PORTAGE_USERNAME and PORTAGE_GRPNAME: https://bugs.gentoo.org/941977

And, I am gonna take the Posix parameter expansion out again. The statement in man make.conf should hold true.

I make this PR a draft as long as the bug isn't resolved.

@duxsco duxsco marked this pull request as draft October 22, 2024 16:23
@duxsco
Copy link
Contributor Author

duxsco commented Oct 22, 2024

fyi, there is also:

local ownership="portage:portage"

@Flowdalic
Copy link
Member

Is this still work-in-progress?

@duxsco
Copy link
Contributor Author

duxsco commented Jan 10, 2025

@Flowdalic As long as https://bugs.gentoo.org/941977 isn't taken care of, this PR cannot be merged.

@thesamesam
Copy link
Member

Just add a default value for now?

@thesamesam
Copy link
Member

Also, portage certainly does default to that - it just may not be exported (can't check easily on mobile).

@duxsco
Copy link
Contributor Author

duxsco commented Jan 11, 2025

fyi, I created a new PR:
#1414

@thesamesam
Copy link
Member

Please rebasd this one now thats in

@thesamesam
Copy link
Member

Needs another rebase and to be marked as ready, if it is.

@duxsco
Copy link
Contributor Author

duxsco commented Jan 15, 2025

Needs another rebase and to be marked as ready, if it is.

OK, I use it for a while on my laptop first.

@thesamesam
Copy link
Member

I think the change looks good, but please:

  1. Split the default fix for user/groups into one commit, and
  2. Split the chown fix into another
    for a total of 2 commits.

Please ideally squash using git rebase -i too.

Both variables default to "portage" according to "man 5 make.conf"
and should be used instead of hardcoding "portage" user.

Bug: https://bugs.gentoo.org/707980
Signed-off-by: David Sardari <[email protected]>
@duxsco duxsco marked this pull request as ready for review January 21, 2025 20:52
@duxsco
Copy link
Contributor Author

duxsco commented Jan 21, 2025

@thesamesam done. I haven't tested the code yet, as I'm busy with private stuff. But if you think it's kosher, feel free to merge.

@thesamesam
Copy link
Member

Thanks.

@thesamesam
Copy link
Member

(I am a bit confused as to how you hadn't tested it yet given the PR was from October and the substance of it was the same, so I hope you just mean the latest variant wasn't yet tested.)

@duxsco
Copy link
Contributor Author

duxsco commented Jan 21, 2025

(I am a bit confused as to how you hadn't tested it yet given the PR was from October and the substance of it was the same, so I hope you just mean the latest variant wasn't yet tested.)

I have tested it at that time. But, I haven't re-tested it recently.

@duxsco
Copy link
Contributor Author

duxsco commented Jan 21, 2025

@thesamesam As this change #1416 isn't in current portage yet, I set just now PORTAGE_USERNAME="portage" and PORTAGE_GRPNAME="portage" in make.conf and did a emerge-webrsync with non-existent /var/db/repos/gentoo. This resulted in the creation of /var/db/repos/gentoo with portage:portage ownership. Without the settings in make.conf, the repo folder gets created with root:root ownership.

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