RPM: Make /usr/bin/code owned by package

[ferdnyc]

Create the /usr/bin/code symlink during %install so that the package owns and manages it. Also, make it relative (for better relocatability).

Before

$ rpm -qf  /usr/bin/code
file /usr/bin/code is not owned by any package

After (simulated)

$ rpm -qf /usr/bin/code
code-1.64.2-1644445844.el7.x86_64

This PR fixes #142906

[ferdnyc]

I realize there are a lot of open PRs in the repo, but is there any chance of getting this reviewed? It looks like @Tyriar is unavailable currently.

It's pretty straightforward, only switcfhing the RPM packaging from creating a symlink during the package transaction process, to creating the symlink and bundling it into the package (so that the package has ownership of that symlink, which isn't the case with the current transaction-script method).

[ferdnyc]

This change should also handily avoid issues like #7350, #9332, etc. that have come up in the past.

[rzhao271]

I'm unsure which directory .. is pointing at. Is there an absolute path one can use instead?

[ferdnyc]

@rzhao271 A relative symlink will be relative to the location of the link itself. So, in this case, the symlink at %{buildroot}/usr/bin/@@NAME@@ will be pointing at %{buildroot}/usr/bin/../share/@@name@@/bin/@@name@@. (Or, normalized, to %{buildroot}/usr/share/@@name@@/bin/@@name@@.)

An absolute path is certainly an option, but it has the downside that the link would be non-relocatable. Using a relative link, the link will already be correct from inside the %{buildroot} (instead of pointing to /usr/bin/ on the build machine), and it will still be good even if someone decides to install the package in /usr/local/, $HOME/.local/, etc.

[ferdnyc]

Common practice is pretty split on the question. On my system, out of 7447 items in /usr/bin/, 950 are symlinks:

• 639 point to other files in the same directory
• 73 point to /etc/alternatives/
• 129 are absolute paths to /usr/
• and 67 are relative paths to either ../{share,lib,libexec,...}, or in a few cases even ../../usr/*.

(There are 42 yet unaccounted for there, out of the total 950. Most (39) are pointing to ./<something> — so, special case of same-directory. The final three outliers, I can't explain. They didn't match any of my greps.)

[ferdnyc]

@rzhao271 I notice that in 7492ba5, @Tyriar changed the scriplet code to use %{_bindir} instead of a hardcoded /usr/bin/, as the location to create the link.

That seems to have been intended as a nod to relocatability, although in practice it isn't because it won't work the way it was apparently intended to. The expansion of %{_bindir} in the scriptlet code is performed by rpmbuild when generating the binary RPM. The scriptlet as embedded into the RPM still contains the same hardcoded /usr/bin/ (assuming that was the value of %(_bindir} on the system creating the RPM), it won't change based on where the package is installed or anything else:

$ rpm --define="_bindir /var/tmp" -E "%{_bindir}/code"
/var/tmp/code

$ rpm --define "_bindir /var/tmp" -q --scripts code
postinstall scriptlet (using /bin/sh):
# Remove the legacy bin command if this is the stable build
if [ "code" = "code" ]; then
	rm -f /usr/local/bin/code
fi

# Symlink bin command to /usr/bin
ln -sf /usr/share/code/bin/code /usr/bin/code

[...]

If the package were actually relocatable, and the symlink could be installed in some other location, then I agree that an absolute-path symlink to /usr/share/... would be more appropriate because the symlink itself might be somewhere other than /usr/bin/. But nothing about the current packaging makes that possible, really, so the %{_bindir} there is a bit odd.

(Actually, it's not, what's odd is that the entire packaging doesn't use macros like %{_bindir}. I would expect the %files section of the specfile to look more like this (with my addition), rather than hardcoding so many of the paths.):

%files
%defattr(-,root,root)
%attr(4755, root, root) %{_datadir}/%{name}/chrome-sandbox

%{_bindir}/%{name}
%{_datadir}/%{name}/
%{_datadir}/applications/%{name}.desktop
%{_datadir}/applications/%{name}-url-handler.desktop
%{_datadir}/mime/packages/%{name}-workspace.xml
%{_datadir}/pixmaps/@@ICON@@.png
%{_datadir}/bash-completion/completions/%{name}
%{_datadir}/zsh/site-functions/_%{name}

If that were the case, then I would absolutely create the symlink to point to %{_datadir}/%{name}/bin/%{name} as there's no way to be sure how %{_bindir} and %{_datadir} are located relative to each other. %{_datadir} may or may not be ../share/, relative to %{_bindir}. But since the current packaging is all absolute paths, their relative locations are fixed and unambiguous.

Still, I'll happily change the relative symlink to an absolute one, if that's preferred.

[rzhao271]

Because it's near the end of the iteration, my plan is to merge this during debt week, which is in two weeks from now. https://github.com/microsoft/vscode/wiki/Development-Process#inside-an-iteration
You can leave the PR as-is. The suggestion to use more macros seems reasonable to me, though I'd put that in a separate PR.

[ferdnyc]

@rzhao271 OK, cool, thanks.

The suggestion to use more macros seems reasonable to me, though I'd put that in a separate PR.

Agreed, though I'm a little stuck on how best to accomplish that, if I wanted to submit such a PR "now" (for small values of "now").

If I were to submit a PR based on the current .spec file, with this PR un-merged, then they'd interact with each other in ways that makes the merge process tenuous. Beyond actual merge conflicts that would have to be resolved (which wouldn't take much effort to sort out), after this PR is merged the other PR would then need to further modify this change, for it to not break the packaging.

In a macro-based specfile, as I mentioned, a symlink targeting a path under %{_datadir} should not assume it's located under ../share/, from %{_bindir}. Installing to macro-driven paths would make a relative-path link inadvisable, unless the relative path from post-expansion %{_bindir} to post-expansion %{_datadir} were first computed for the link.

(Actually, the relative path link is irrelevant. The same issue comes up with absolute paths, since linking to /usr/share/... would no longer be correct once the target file is being installed to %{_datadir}/....

I suppose I could create a PR that branches off of this PR's commit, and add a note that this PR should be merged prior to that one, but that feels a bit... well, either "convoluted", or "presumptuous" on my part. If not both.

The other option is to wait until after this PR is merged, to start work on the other one, since it would then be able to take this change into account. But (in addition to presenting a challenge for my attention span, if I'm to remember
that I should revisit something I need to work on "some time after two weeks from now"), IIUC that would mean delaying that change until the following release? Which isn't really a problem, as there's certainly no rush, but it
does make the process feel a bit drawn out.

Any suggestions you have for how best to proceed would be welcome.

[Tyriar]

@ferdnyc branching off the original PRs commit is good since it will just turn into a regular single change PR once this gets merged.