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

combined use of ID and ID_LIKE #372

Closed
wants to merge 1 commit into from
Closed

Conversation

kewlfft
Copy link
Contributor

@kewlfft kewlfft commented Sep 30, 2018

Signed-off-by: Xrjy SSG xrjy@nygb.rh.bet (rot13)

bin/etc-update Outdated
@@ -33,12 +33,13 @@ get_config() {
}

OS_RELEASE_ID=$(source /etc/os-release >/dev/null 2>&1; echo "${ID}")
OS_RELEASE_ID_LIKE=$(source /etc/os-release >/dev/null 2>&1; echo "${ID_LIKE}")
Copy link
Contributor

Choose a reason for hiding this comment

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

You can extract multiple variables from a sourced file in this way:

eval "$(source /etc/os-release &>/dev/null; declare -A OS_RELEASE_DATA=([ID]="${ID}" [ID_LIKE]="${ID_LIKE}"); declare -p OS_RELEASE_DATA)"

And then use "${OS_RELEASE_DATA[ID]}" and "${OS_RELEASE_DATA[ID_LIKE]}".

So either directly case ":${OS_RELEASE_DATA[ID]}:${OS_RELEASE_DATA[ID_LIKE]}:" in or assign OS_RELEASE_ID="${OS_RELEASE_DATA[ID]}" and OS_RELEASE_ID_LIKE="${OS_RELEASE_DATA[ID_LIKE]}" and use case ":${OS_RELEASE_ID}:${OS_RELEASE_ID_LIKE}:" in.

Copy link
Member

Choose a reason for hiding this comment

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

Don't abuse eval. Ever.

Copy link
Contributor

Choose a reason for hiding this comment

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

My solution avoids spawning 2 subprocesses and opening /etc/os-release twice for reading.

It is also possible to send contents of variables to temporary files, but that only avoids subprocess problem.

So there is no consensus that this would be any abuse of eval.

Copy link
Member

Choose a reason for hiding this comment

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

There are many solutions to every problem, and the one using eval is almost always wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Here's a way to read the file once, without using eval:

for var in OS_RELEASE_ID OS_RELEASE_ID_LIKE; do
	read -r -d '' || break
	declare ${var}="${REPLY}"
done < <(source /etc/os-release >/dev/null 2>&1; printf -- '%s\0%s\0' "${ID}" "${ID_LIKE}")

Copy link
Member

Choose a reason for hiding this comment

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

You're all sort of missing the forest for the trees. Not only is eval (a dangerous thing which should only be used with care, and this doesn't qualify) completely unnecessary, you don't need two variables to begin with.

OS_RELEASE_POSSIBLE_IDS=$(source /etc/os-release >/dev/null 2>&1; echo "${ID}:${ID_LIKE}")

case ":${OS_RELEASE_POSSIBLE_IDS}:" in

The variables are used once, right here and now. They don't need to be uniquely available. No read loop hacks, no frivolous use of eval, no opening the file twice for reading, no forking bash into a second subshell on top of the first.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yes, now I see that the consumer joins them with a colon anyway, so it makes sense to join them in the subshell.

@kewlfft
Copy link
Contributor Author

kewlfft commented Oct 3, 2018

Implementation based on @eli-schwartz proposal, is there anything else needed?

bin/etc-update Outdated
fedora|rhel) OS_FAMILY='rpm' ;;
arch|archarm|arch32|manjaro|antergos) OS_FAMILY='arch' NEW_EXT='pacnew';;
*) OS_FAMILY='gentoo' ;;
case "${OS_RELEASE_POSSIBLE_IDS}" in
Copy link
Member

Choose a reason for hiding this comment

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

Please omit quotes for case statements, since they are unnecessary, just like in [[.

@zmedico
Copy link
Member

zmedico commented Oct 3, 2018

This looks good, please remove the case statement quotes and squash all commits into one.

@kewlfft
Copy link
Contributor Author

kewlfft commented Oct 3, 2018

We should be good, how do I squash all commits now?

@eli-schwartz
Copy link
Member

eli-schwartz commented Oct 3, 2018

Signed-off-by: Kewl Fft xrjy@nygb.rh.bet (rot13)

combined use of ID and ID_LIKE
Signed-off-by: Kewl Fft xrjy@nygb.rh.bet (rot13)
@kewlfft
Copy link
Contributor Author

kewlfft commented Oct 3, 2018

Thanks @eli-schwartz , only one commit now

@kewlfft
Copy link
Contributor Author

kewlfft commented Oct 4, 2018

@zmedico is it good for merging or do you need anything from me? thanks

Copy link
Member

@zmedico zmedico left a comment

Choose a reason for hiding this comment

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

Looks good, I planning to merge things this weekend and make a release.

@gentoo-bot gentoo-bot closed this in d5ed2a3 Oct 6, 2018
@zmedico
Copy link
Member

zmedico commented Oct 6, 2018

Merged in d5ed2a3, thanks!

@@ -32,13 +32,13 @@ get_config() {
"${PORTAGE_CONFIGROOT}"etc/etc-update.conf)
}

OS_RELEASE_ID=$(cat /etc/os-release 2>/dev/null | grep '^ID=' | cut -d'=' -f2 | sed -e 's/"//g')
OS_RELEASE_POSSIBLE_IDS=$(source /etc/os-release >/dev/null 2>&1; echo ":${ID}:${ID_LIKE}:")
Copy link
Member

Choose a reason for hiding this comment

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

I originally overlooked mentioning, that ID_LIKE can contain multiple, space-separated IDs. So this would really need to be ${ID_LIKE/ /:} or it would not detect any ID_LIKE that has multiple associations.

Copy link
Contributor Author

@kewlfft kewlfft Oct 9, 2018

Choose a reason for hiding this comment

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

Good point, then ${ID_LIKE/ /:} will only separate the first 2 IDs with :, what about if there are 3 of them?
I think we want ${ID_LIKE// /:} here, do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

…or you could just, like, use space as the separator and then you wouldn't have to replace anything.

Copy link
Member

@eli-schwartz eli-schwartz Oct 9, 2018

Choose a reason for hiding this comment

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

And then also quote the part in the case statement, then be personally responsible for making sure no one ever accidentally tries to optimize the leading and trailing spaces out of OS_RELEASE_POSSIBLE_IDS. But sure.

I'd personally avoid using spaces as a glob pattern match delimiter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With spaces, the case patterns become quite heavy and more prone to errors.

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.

5 participants