Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

appendScript to .bashrc for Ubuntu-based #50

Closed
wants to merge 4 commits into from

Conversation

edsfocci
Copy link

@edsfocci edsfocci commented Jan 7, 2017

This change is due to Ubuntu's preference to use .bashrc for sourcing executables. It works better for me, and I don't need to log out to be able to use avn. I also re-worked the promises, which was necessary for my edit, but most of the original code's untouched, and should still work like before this change.

@coveralls
Copy link

coveralls commented Jan 7, 2017

Coverage Status

Coverage decreased (-6.2%) to 93.802% when pulling 820e4ee on edsfocci:add-bashrc-to-setup into 24e1b1d on wbyoung:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-6.2%) to 93.802% when pulling 820e4ee on edsfocci:add-bashrc-to-setup into 24e1b1d on wbyoung:master.

@edsfocci
Copy link
Author

edsfocci commented Jan 7, 2017

All the minor bugs are fixed. I don't know if your team has any philosophical or logical reasons to append the sourcing script to .bash_profile instead of .bashrc; feel free to enlighten me. I know that my Linux Mint 18 never came with .bash_profile in my home folder, and probably for a very good reason. Anyways, I was able to move the contents of the .bash_profile (1 line) that your module creates, to the bottom of my .bashrc and it works great. This fix is to automate that process for Ubuntu users only, to minimize conflicts with other operating systems.

@wbyoung
Copy link
Owner

wbyoung commented Jan 9, 2017

Thanks for the work on this, @edsfocci.

I really do want this to be easier on everyone, but before this PR is merged, I want to ensure that it's being fixed for the long term. See also: #31 where there's been some discussion including pointing out that getting this just right is actually quite difficult.

Here's what I'd want to know before accepting this PR:

  • What do other projects do (links to lines of code) that write to bash init scripts?
  • What is created by default on various platforms (Ubuntu, Fedora, Mac OS X would be my top 3) — .bashrc, .bash_profile or both and does one source the other if they're both present?
  • When is each script run on these platforms? For instance, I know that Mac OS X is the exception here where it runs .bash_profile for non-login shells.

After a quick glance at the code, the main thing I'd like to see different is to default to the Linux way which is probably more likely to be "standard" and have a special case for Mac OS X (darwin).

A whole can of worms gets opened as we start to consider both files, though, since one can source the other. Usually, if one is sourcing the other, we wouldn't want to edit that file. We'd want to edit the one that's not sourced.

I think the algorithm could be:

  • If one file exists, use that.
  • If both exist, does one source the other? If so, use the source-e not the source-r.
  • If neither of the above cases is true, then use the "platform" file. i.e. .bash_profile for darwin and .bashrc for everything else.

@edsfocci
Copy link
Author

Ok, I made a pull request in a different branch, made according to your algorithm. I'm not sure if you want to do anything still with .zshrc (quick Google search shows that ArchLinux might use .zshrc). Also, in issue #49, the guy uses .profile in his Mac computer.

Working on your checklist will probably take some time. I can vouch for Linux Mint 18 (related to Ubuntu 16.04), and getting info on other Linux distros will probably take time (probably spend a few weekends). I won't be able to work with Mac OS X versions since I don't have a Mac unfortunately.

I'll probably work some more on this on the next couple of days.

@wbyoung
Copy link
Owner

wbyoung commented Jan 15, 2017

@edsfocci awesome, I'll continue the conversation over on #51.

@wbyoung wbyoung closed this Jan 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants