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

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

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

wants to merge 6 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 •
edited
Loading

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
emerge-webrsync: Use PORTAGE_USERNAME and PORTAGE_GRPNAME (default: p…
157a7ad 
…ortage)

Both variables default to "portage" according to "man make.conf".
But, this isn't the case. Therefore, I make use of Posix parameter expansion.

Signed-off-by: David Sardari <[email protected]>
@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
floppym
floppym requested changes Oct 22, 2024
View reviewed changes
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 •
edited
Loading

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
emerge-webrsync: Remove Posix parameter expansion and add "die" function
866239b 
Both variables default to "portage" according to "man make.conf".
This statement should hold true. Therefore, the parameter expansion is removed.

Signed-off-by: David Sardari <[email protected]>
@duxsco
emerge-webrsync: Add "die" function after mkdir
edbceb2 
Signed-off-by: David Sardari <[email protected]>
@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:

portage/misc/emerge-delta-webrsync

Line 524 in fad3818

local ownership="portage:portage"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floppym floppym floppym requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@duxsco @thesamesam @floppym