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

Add UI helpers to .deb package & change install location w/ symlinking #405

Merged
merged 2 commits into from
Aug 17, 2021

Conversation

vdye
Copy link
Contributor

@vdye vdye commented Aug 5, 2021

Fixes #387

Changes

  • Add Atlassian.Bitbucket.UI and GitHub.UI to created .deb package
  • Move the binary install location to /usr/local/bin/share/gcm-core, with symlinks in the original location (/usr/bin)

Questions

  • Is /usr/local/bin/share/gcm-core the right install directory to use?
    • Answer: use /usr/local/share/gcm-core, symlinked to /usr/local/bin
  • Right now the symlinks are relative paths (see here) - should they be absolute path links instead?

@mjcheetham mjcheetham self-requested a review August 9, 2021 08:58
@vdye
Copy link
Contributor Author

vdye commented Aug 9, 2021

I'm not sure why the build failed - the only changes in this pull request are for Packaging.Linux, so it shouldn't affect the Mac test execution (link to test failure). Is this a test that occasionally fails, or is this change interacting with the tests somehow?

@mjcheetham
Copy link
Collaborator

mjcheetham commented Aug 9, 2021

Is this a test that occasionally fails, or is this change interacting with the tests somehow?

I've not seen this failure in CI before, but I've heard of the same underlying issue affecting some customers who use the Keychain from other C# projects, but only sporadically.

@mjcheetham
Copy link
Collaborator

@vdye I've pushed a commit (1cefe59) that should help side-step the flakey Keychain test for macOS. If you rebase this work on the latest main branch, you should be good!

@vdye vdye force-pushed the 387-add-ui-and-symlink-deb branch 2 times, most recently from 957094a to 5544882 Compare August 10, 2021 12:21
@vdye vdye force-pushed the 387-add-ui-and-symlink-deb branch from 5544882 to d87a3b8 Compare August 10, 2021 12:58
@mjcheetham
Copy link
Collaborator

Is /usr/local/bin/share/gcm-core the right install directory to use?

We had a similar question when first putting the Linux support in: to use /usr or /usr/local. In the end we opted for /usr (even though on macOS we're using /usr/local).

See https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch04.html

Re-reading the FHS I'd argue that /usr/local should be the correct place, and we're currently placing bits in the wrong place:

4.9.1. Purpose
The /usr/local hierarchy is for use by the system administrator when installing software locally. It needs to be safe from being overwritten when the system software is updated. It may be used for programs and data that are shareable amongst a group of hosts, but not found in /usr.

Locally installed software must be placed within /usr/local rather than /usr unless it is being installed to replace or upgrade software in /usr.

Thoughts @derrickstolee @jeffhostetler @dscho ?

@derrickstolee
Copy link
Contributor

Re-reading the FHS I'd argue that /usr/local should be the correct place, and we're currently placing bits in the wrong place:

4.9.1. Purpose
The /usr/local hierarchy is for use by the system administrator when installing software locally. It needs to be safe from being overwritten when the system software is updated. It may be used for programs and data that are shareable amongst a group of hosts, but not found in /usr.
Locally installed software must be placed within /usr/local rather than /usr unless it is being installed to replace or upgrade software in /usr.

I agree that /usr/local/ is the right place. That's where we put microsoft/git on install, too.

The biggest question is: how do we get users who currently have it in /usr out of their current state? /usr might take priority, so if we simply start adding it to /usr/local we will have those users stuck on an older version. (Right?)

@mjcheetham
Copy link
Collaborator

mjcheetham commented Aug 10, 2021

/usr might take priority, so if we simply start adding it to /usr/local we will have those users stuck on an older version. (Right?)

I believe you'd want /usr/local to take priority over /usr. Logically your local install should take precedence over any distribution binaries, no (hence why we put microsoft/git there)?

I know on macOS the default PATH is /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin.

In lieu of more concrete defaults docs, here's my findings of default PATH values for various distros:

Distribution Version Default PATH
Alpine 3.14 /sbin:/usr/sbin:/bin:/usr/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
CentOS 8 /home/$USER/.local/bin:/home/$USER/bin:/usr/local/bin:/usr/local/sbin:/usr/bin:/usr/sbin
openSUSE Leap 15 /home/$USER/bin:/usr/local/bin:/usr/bin:/bin
Ubuntu 18.04 /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin

Using the getconf tool on Ubuntu 18.04 however you get this:

> getconf PATH
/bin:/usr/bin

..which is the opposite, and doesn't list local at all(!)

@jeffhostetler
Copy link

On my Mac:

$ getconf PATH
/usr/bin:/bin:/usr/sbin:/sbin

whereas my $PATH has (somehow) accumulated /usr/local/bin.

@mjcheetham
Copy link
Collaborator

whereas my $PATH has (somehow) accumulated /usr/local/bin.

Mac has the path_helper tool and its additional way of setting up the PATH.

I really don't think there is anything akin to an default ordering, defacto or otherwise. 😢

@vdye vdye force-pushed the 387-add-ui-and-symlink-deb branch from d87a3b8 to 6b6b08b Compare August 11, 2021 18:17
@vdye vdye marked this pull request as ready for review August 11, 2021 20:37
@vdye vdye requested a review from mjcheetham August 11, 2021 20:37
Copy link
Collaborator

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

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

:shipit:

@mjcheetham mjcheetham merged commit 8676ca5 into git-ecosystem:main Aug 17, 2021
@mjcheetham mjcheetham mentioned this pull request Oct 8, 2021
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.

Debian package is missing new UI helper executables
4 participants