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

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

Merged
merged 8 commits into from
Jul 6, 2022
Merged

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

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0900570
RPM: Make /usr/bin/code owned by package
ferdnyc Feb 12, 2022
07f41ae
Merge branch 'main' into patch-1
ferdnyc Feb 24, 2022
5e51c3b
Merge branch 'main' into patch-1
ferdnyc Mar 19, 2022
f7693f3
Merge branch 'main' into patch-1
ferdnyc Apr 27, 2022
4ac2575
Merge branch 'main' into patch-1
ferdnyc May 9, 2022
34c5a25
Merge branch 'main' into patch-1
rzhao271 Jun 21, 2022
c1cc94b
Merge branch 'main' into patch-1
ferdnyc Jun 22, 2022
7ea9944
Merge branch 'main' into patch-1
ferdnyc Jun 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions resources/linux/rpm/code.spec.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Visual Studio Code is a new choice of tool that combines the simplicity of a cod
%define _build_id_links none

%install
mkdir -p %{buildroot}/usr/bin
mkdir -p %{buildroot}/usr/share/@@NAME@@
mkdir -p %{buildroot}/usr/share/applications
mkdir -p %{buildroot}/usr/share/pixmaps
Expand All @@ -32,16 +33,14 @@ cp -r usr/share/mime/packages/@@NAME@@-workspace.xml %{buildroot}/usr/share/mime
cp -r usr/share/pixmaps/@@ICON@@.png %{buildroot}/usr/share/pixmaps
cp usr/share/bash-completion/completions/@@NAME@@ %{buildroot}/usr/share/bash-completion/completions/@@NAME@@
cp usr/share/zsh/site-functions/_@@NAME@@ %{buildroot}/usr/share/zsh/site-functions/_@@NAME@@
ln -s ../share/@@NAME@@/bin/@@NAME@@ %{buildroot}/usr/bin/@@NAME@@
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@ferdnyc ferdnyc Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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


%post
# Remove the legacy bin command if this is the stable build
if [ "@@NAME@@" = "code" ]; then
rm -f /usr/local/bin/code
fi

# Symlink bin command to /usr/bin
ln -sf /usr/share/@@NAME@@/bin/@@NAME@@ %{_bindir}/@@NAME@@

# Register yum repository
# TODO: #229: Enable once the yum repository is signed
#if [ "@@NAME@@" != "code-oss" ]; then
Expand All @@ -56,17 +55,14 @@ ln -sf /usr/share/@@NAME@@/bin/@@NAME@@ %{_bindir}/@@NAME@@
update-mime-database /usr/share/mime &> /dev/null || :

%postun
if [ $1 = 0 ]; then
rzhao271 marked this conversation as resolved.
Show resolved Hide resolved
rm -f /usr/bin/@@NAME@@
fi

# Update mimetype database for removed workspace mimetype
update-mime-database /usr/share/mime &> /dev/null || :

%files
%defattr(-,root,root)
%attr(4755, root, root) /usr/share/@@NAME@@/chrome-sandbox

/usr/bin/@@NAME@@
/usr/share/@@NAME@@/
/usr/share/applications/@@NAME@@.desktop
/usr/share/applications/@@NAME@@-url-handler.desktop
Expand Down