Page MenuHomeFreeBSD

Fix install-missing-packages
ClosedPublic

Authored by 0mp on Feb 3 2020, 4:59 PM.

Details

Summary
Fix install-missing-packages

r519284 introduced a new target, install-missing-packages, which is meant to
allow users to install dependencies via pkg(8) instead of building them
themselves locally.

The target was producing errors when the dependencies were already available on
the system. This commit adjusts this behavior to just silently do nothing if
everything is fine.

Also, remove some trailing whitespace introduced in the original commit.

Other considerations:

  • In the past users were using one-liners like make missing | xargs pkg install --automatic --yes to install dependencies from packages (as documented in ports(7)). Unfortunately, the both the implementation of install-missing-packages and the one-liner install all the packages associated with the origin, which is usually not desired. I wonder if there is a way to, for example, install only the required flavor of a Python port instead of all the available ones when installing a Python dependency this way.
  • The manual page for ports has to be updated. I'll do it soon.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

0mp created this revision.Feb 3 2020, 4:59 PM
bapt added a comment.Feb 3 2020, 5:03 PM

Actually it should also be surrounded by SU_CMD while you are at it ;)

0mp added a comment.Feb 3 2020, 5:07 PM
In D23484#515324, @bapt wrote:

Actually it should also be surrounded by SU_CMD while you are at it ;)

Sure!

0mp updated this revision to Diff 67722.Feb 3 2020, 5:09 PM

Wrap pkg call with SU_CMD

0mp planned changes to this revision.Feb 3 2020, 5:22 PM
  1. We cannnot use xargs here as it breaks interactive input.
  2. SU_CMD cannot be used this way.
mat added a comment.Feb 3 2020, 5:51 PM

I do not fully understand what this is fixing. It looks like you are just moving things around to use a pipe instead of a temporary variable.

0mp updated this revision to Diff 67766.Feb 4 2020, 8:45 AM
  • Revert to the original implementation of install-missing-packages.
  • Do not call pkg-install(8) if there is nothing to install.
  • Wrap the pkg command with SU_CMD for better user experience.

@mat, you are somewhat right, haha. I thought that using xargs is going to be an elegant way of covering the case when there is nothing to install (no origins being passed to pkg-install(8)). It quickly turned out that I broke the interactivity of this command.

Now I reverted to the original implementation and improved it. Sorry for the noise.

0mp updated this revision to Diff 67767.Feb 4 2020, 8:46 AM
  • Add a missing period at the end of the description of the target.
bapt accepted this revision.Feb 4 2020, 9:25 AM
This revision is now accepted and ready to land.Feb 4 2020, 9:25 AM
This revision was automatically updated to reflect the committed changes.
mat added a comment.EditedFeb 4 2020, 12:14 PM

Points 1, 2 and 5 of https://en.wikipedia.org/wiki/The_Elements_of_Programming_Style :

  1. Write clearly -- don't be too clever.
  2. Say what you mean, simply and directly.
  1. Write clearly -- don't sacrifice clarity for efficiency.
head/Mk/bsd.port.mk
4335–4336

Ok, so, this is wrong.
The correct way to write it is:

if [ -n "$${_dirs}" ]; then \
    ${SU_CMD}...; \
fi

Because:

  1. using [ ] || is akin to using a unless or if !, but worse
  2. if you are using if ! [ -z might as make it easier to read, if [ -n.
adamw added a subscriber: adamw.Feb 4 2020, 12:55 PM
adamw added inline comments.
head/Mk/bsd.port.mk
4336

Also, you have quotes within quotes. It'll break if PORTSDIR has a space in it. Probably better to turn the sed arg into single quotes.

0mp added inline comments.Feb 6 2020, 9:37 AM
head/Mk/bsd.port.mk
4335–4336

@mat

You are right. Thank you for the suggestion. I'll send another patch shorty.

4336

@adamw,

hmmm, I think that this is not a problem because the second pair of quotes is inside a subshell.

For example, the following code works as I expect:

export VAR="foo bar"
ls "$(echo "/foo bar" | sed "s/$VAR//g")"
# Expected result is to run "ls /"

Am I missing something?

adamw added inline comments.Feb 6 2020, 1:09 PM
head/Mk/bsd.port.mk
4336

You are correct. I missed the subshell there. Thanks for setting me straight!