search

andsens / homeshick

git dotfiles synchronizer written in bash
MIT License
2.11k stars 145 forks source link

Make homeshich.sh position-independent #136

Closed oreinert closed 9 years ago

oreinert commented 9 years ago

The current implementation of homeshick.sh contains a hard-coded path to the installation location of homeshick. This patch makes homeshick position-independent by computing the actual installation location at the time homeshick.sh is sourced (e.g., by .bashrc).

Users should never notice any difference if they install homeshick as described in the wiki. Hence, there are no specific tests to ensure the change works for arbitrary installation locations, but all existing tests show that it works for the default location.

oreinert commented 9 years ago

The purpose of this change is to make packaging of homeshick possible without applying patches: [https://build.opensuse.org/package/show/home:oreinert/homeshick]

andsens commented 9 years ago

Sweet! It's great to see homeshick getting this level of attention :-)

If people are running fish or csh, they're going to have a bad time though. You didn't patch those files...

oreinert commented 9 years ago

You're right about the csh and fish scripts - let me fix those.

oreinert commented 9 years ago

OK, so I fixed homeshick.fish. However, I can't figure out a way to fix homeshick.csh, so I decided to leave it as-is. That should not be a problem, it will continue to work as expected for users who install homeshick by cloning.

For the RPM I will patch homeshick.csh to point at the proper installation location. That's OK because it isn't needed for running test cases.

andsens commented 9 years ago

Hence, there are no specific tests to ensure the change works for arbitrary installation locations, but all existing tests show that it works for the default location.

Well, they are also useful to ensure that I don't break your modifications in the future :-)

No matter, I'll find the time to go through it tonight, merge it and add some tests myself.

oreinert commented 9 years ago

I'm having some reservations with this PR. Scripts that have to detect their own location is a bad smell, somehow - it all seems a bit too convoluted for my taste. How about if we just define that an environment variable (e.g., HOMESHICK_DIR) defines the location of homeshick? The sourced scripts can define a default value for it, and homeshick itself can simply require that it's defined. (In a way, this is, sort of, what line 4 in bin/homeshick is all about, anyway.) That would make the things this PR tries to do much simpler. I'm willing to code it up.

andsens commented 9 years ago

I agree about the smell, but to be honest, getting my code shipped as a package the second time lowered my standards a little :-)

The sourced scripts can define a default value for it, and homeshick itself can simply require that it's defined.

Why not keep the default in bin/homeshick instead? This way the executable also works when invoked directly (albeit without the cd command).

Also: You might want to ping the guys over at archlinux, not quite sure how they solved it, but this solution seems great.

oreinert commented 9 years ago

So, I opened PR #138 as an alternative to this one. It's not quite complete yet, but it shows the direction I'd like it to go. Feel free to decline this PR if you agree #138 is a better approach. It (also) keeps the default in bin/homeshick, as you suggested - that's a good idea. The ArchLinux package does the same as my current package, it patches the scripts. When packaging homeshick it makes sense to change the default location by patching; the HOMESHICK_DIR override is needed for running the tests during the build.

andsens commented 9 years ago

Completely agree :-)

Closing this one...