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

Install scale to user home #17

Merged
merged 6 commits into from
Mar 15, 2017

Conversation

atykhonov
Copy link
Contributor

This PR changes the default installation directory from /usr/local/ to $HOME/.scale. Two executable links (scale and scale-ui) are installed in /usr/local/bin/ in the same way as it was.

I was thinking about usage of $HOME/.config/scale/ and $HOME/.config/pharo/ directories (instead of $HOME/.scale) accordingly to XDG specification. But most of the files which reside in these directories are executables (not user specific configuration files). So I decided to use $HOME/.scale. Please, let me know if $HOME/.config folder would be more appropriate.

Thank you!

@cdlm
Copy link
Collaborator

cdlm commented Nov 16, 2016

Installing Scale under $HOME but executables in a global place seems contradictory.
What's so bad about requiring the user to setup their $PATH?

Also, /usr/local is an acceptable install prefix in many cases. I think the install script should be configurable to install Scale in a user-specified prefix, and possibly to rely on an independently installed Pharo; that would cut most of the work for #14.

@LucFabresse
Copy link

I agree with Damien.
Install it either in /usr/local or in $HOME/.scale.
But we should not mix and the user must set his PATH in the second case.

@atykhonov
Copy link
Contributor Author

atykhonov commented Mar 15, 2017

Hello @cdlm @LucFabresse !

Sorry for the long delay and thank you very much for your review! You are right, I totally agree with you. So, I've changed everything so that scale is installed into $HOME/.scale only.

Please review.

@LucFabresse
Copy link

Hi,

Thanks for this. I think it is better.
Just one question: why did you put it in HOME/.config and not HOME/.scale directly?

Luc

@guillep
Copy link
Owner

guillep commented Mar 15, 2017

Hi @LucFabresse, by reading the diff, I do not see he is using .config anymore. However, there is still the example file pwd.st that prints .config.

My main question is why removing the install.st file. I understand you could replace it with a rm -rf, but

  • the script was only used in setupScale.sh file, but it was meant to be a part of scale's API. In other words, if you want to uninstall scale, you call the uninstall script.
  • in case scale grows in complexity I'd like to have a separate script, even if it just does so far a rm -rf inside.
  • I would like it to be written in scale. If we port scale to windows in the future (which is not so crazy to think about), a rm -rf solution will not work

@guillep
Copy link
Owner

guillep commented Mar 15, 2017

BTW, related to this, I created issue #20. We should add in our test suite that scale is correctly uninstalled.

@LucFabresse
Copy link

LucFabresse commented Mar 15, 2017 via email

@atykhonov
Copy link
Contributor Author

My main question is why removing the install.st file. I understand you could replace it with a rm -rf, but

The main reason why I removed uninstall.st is it doesn't work well. After it finishes execution of all code in itself, Pharo' virtual machine throws an error. You could try this by reverting uninstall.st and executing it. I have no idea why this error happens and how to resolve it. Any help would be highly appreciated!

@guillep
Copy link
Owner

guillep commented Mar 15, 2017 via email

@atykhonov
Copy link
Contributor Author

atykhonov commented Mar 15, 2017

I think, ok, no problem. I've reverted the commits that relate to removing of uninstall.st. I created issue #21 regarding the error which occurs while uninstallation.

@atykhonov
Copy link
Contributor Author

Btw, uninstallation does its job. What is wrong, is that the error outputs to the console, so that that could be a little bit confusing for a user. Hi might think that uninstallation went wrong.

Does the changes looks good now?

@guillep guillep merged commit c9fc84a into guillep:master Mar 15, 2017
@guillep
Copy link
Owner

guillep commented Mar 15, 2017

I discussed with @sbragagnolo and it looks ok. Thanks!

@guillep
Copy link
Owner

guillep commented Mar 15, 2017

Somehow, this broke the build.

https://travis-ci.org/guillep/Scale/builds/211441475

Which is strange becase the validation against the PR was ok (https://travis-ci.org/guillep/Scale/pull_requests)

@atykhonov
Copy link
Contributor Author

It fails because (I'm looking at https://travis-ci.org/guillep/Scale/jobs/211441477) scale could not be found.

It seems that travis job needs to be adjusted, i.e. path to scale needs to be added to PATH.

@atykhonov
Copy link
Contributor Author

I suppose that between these two lines:

219 $ sudo ./setupScaleDev.sh
323 $ ./examples/lsst.st

we need export PATH="$HOME/.scale/scale:$PATH".

@guillep
Copy link
Owner

guillep commented Mar 15, 2017 via email

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.

4 participants