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

Allow installation via package managers #39

Merged
merged 17 commits into from
Sep 4, 2021
Merged

Conversation

jxa
Copy link

@jxa jxa commented Jul 26, 2021

Purpose and Motivation

SuperCollider currently bundles language modes for emacs, in addition to vim, gedit, etc. This is convenient for users installing from source, but this convenience comes with a cost.

Package managers for emacs have come a long way in the last 10+ years since the decision was made to bundle language modes with SC. Today emacs users have an expectation that they can install packages via a couple of standard methods, usually a package on MELPA or direct from a git repo via straight.el.

Changes

The strategy implemented here is to use native package managers for both emacs and supercollider. Once this PR is merged we can publish a package on melpa. Instructions are given for another popular emacs package manager.

On the flip-side, the supercollider code will be distributed via Quarks. This PR adds a quark file and installation instructions.

We continue to support installation via make, though the mechanism had to change slightly. When compiling with cmake the sclang-vars.el file will be written, pointing to the proper data directory. When installed via package manager, the config it will guess the proper location by looking at the system-type, but it can be customized if it's not correct.

Testing

I have verified the installation on MacOS (version 11.4) and on debian linux. On MacOS I followed the new README instructions, but substituted the following package definition because this PR isn't merged yet

(package! sclang :recipe (:host github :repo "jxa/scel" :branch "packaging" :files ("el/*.el")))

On debian linux I installed supercollider from source, pointing the editors/sc-el submodule to this branch. Both halves of the code were installed properly and I was able to boot sclang and scserver from inside emacs (after requiring the library)

jxa added 2 commits July 25, 2021 19:54
Only when compiling with cmake will sclang-vars.el be written. This
commit moves relevant constants into customizable vars. Only if the
`sclang-system-data-dir` exists is it respected, otherwise it will guess
the proper location by looking at the system-type.

The purpose of this is to make it possible to build and distribute a
package independent of the SuperCollider build process, while ensuring
that those users who have become accustomed to the current install
process are not disrupted.
@jxa jxa changed the title Distribution: Allow installation via package managers Allow installation via package managers Jul 26, 2021
@jxa jxa force-pushed the packaging branch 3 times, most recently from e845152 to e92aa58 Compare July 27, 2021 16:08
@jxa jxa marked this pull request as ready for review July 27, 2021 16:16
@jxa
Copy link
Author

jxa commented Jul 27, 2021

@dyfer and @joshpar this is now ready for review

@dyfer
Copy link
Member

dyfer commented Jul 27, 2021

Thanks @jxa I'll try to test this soon.
Some comments questions so far:

  • do I understand correctly that after merging this, we need to 1) update sc-el in the main sc repo and 2) add this to MELPA? anything else we need?
  • maybe the part in the README about MELPA should wait until we get the package in there? making that change later will be quick/easy to merge and it may avoid some confusion in the time frame between accepting this and getting the package into MELPA; OR we merge as is and give ourselves a short time frame to get the package into MELPA and if this doesn't happen in that time, you'd make another PR to make a note that the package is not yet at MELPA... what do you think?

@jxa
Copy link
Author

jxa commented Jul 27, 2021

Good point about the README @dyfer. I think it makes sense to pull out the bit about MELPA into a follow-up PR, which can be merged after the package is published. I'll make that change tonight.

do I understand correctly that after merging this, we need 1) update sc-el in the main sc repo and 2) add this to MELPA? anything else we need?

As far as I know those are the only two

@jxa
Copy link
Author

jxa commented Jul 28, 2021

OK I pulled the documentation change out into #40.

Copy link

@bjornmossa bjornmossa left a comment

Choose a reason for hiding this comment

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

Code looks good. Did you try to run it on vanilla Emacs?

@jxa
Copy link
Author

jxa commented Aug 4, 2021

Code looks good. Did you try to run it on vanilla Emacs?

I have run it in a fresh debian VM, compiled from source, and in an existing install of emacs on my main machine (doom config), after uninstalling my existing setup. It's not currently possible to install with package-install because the package doesn't exist on melpa yet and it won't until this PR is merged.

@jxa
Copy link
Author

jxa commented Aug 4, 2021

@dyfer how do you feel about merging at this point? I commit to seeing this change through by publishing to melpa and getting the docs updated. If there are some bugs that come up on certain systems as a result of publishing I can work to iron those out too.

Any risk of disruption on the compiled / linux side of things can be mitigated by delaying the supercollider/editors/scel submodule until we get some feedback (or spend some time with no bug reports)

Anything else I should be thinking about?

@dyfer
Copy link
Member

dyfer commented Aug 5, 2021

@jxa I just want to test this myself before merging - should get to it sometime this week. Sorry for the delay!

@dyfer
Copy link
Member

dyfer commented Aug 6, 2021

Hi @jxa
Sorry - I'm not very familiar with Emacs and I'm having problems testing this.
After I follow https://github.com/raxod502/straight.el to install straight.el (putting the boostrap code in my init file), when I try to run (package! sclang :recipe (:host github :repo "jxa/scel" :branch "packaging" :files ("el/*.el"))) I am getting Debugger entered--Lisp error: (void-function package!). Again, I haven't used Emacs (Aquamacs) in a very long time and I'm probably missing something basic... would you mind pointing me in the right direction here? Do I actually need https://github.com/raxod502/straight.el to install this package from your fork?

As for readme, sorry for not being clear.
I think the readme should be a part of this PR, but for now it should not reference MELPA, but state how to install the package before it gets to MELPA, i.e. (package! sclang :recipe (:host github :repo "supercollider/scel" :files ("el/*.el"))) or whatever the correct command is. After the package gets into MELPA, you would make another PR amending that section of the readme to include information about MELPA.

Looking at the readme in #40 I'd also suggest to start the section about the quark with the warning that it should not be installed if the sc-el mode was installed in a traditional manner (when building or using system package like supercollider-emacs). Otherwise users might install it on top of already present sc-el classes and that leads to duplicate classes and class library not compiling.

@jxa
Copy link
Author

jxa commented Aug 6, 2021

Hi @dyfer I'm really glad you decided to check it out because it forced me to discover a new bug :)

The issue you have installing with straight.el is due to some bad instructions I provided. I'm using doom emacs, which has that package! wrapper function, but the way you can install without doom using straight.el is

(straight-use-package
 '(sclang :type git :host github :repo "jxa/scel" :branch "packaging" :files ("el/*.el")))

This is what exposes the bug. When installed this way, it looks like some files aren't being required properly so I get the error (void-function sclang-make-buffer-name).

I have some time this weekend which I can use to sort out this new issue and update the readme. Thanks again, and sorry for wasting your time with imprecise instructions!

jxa added 2 commits August 9, 2021 11:57
The issue manifests when sclang is compiled. Steps to reproduce in real
emacs config:
1. Install the package and compile it
2. Open a file with a `.sc` or `.scd` extension
3. Witness the error `(void-function sclang-make-buffer-name)`

Steps to reproduce with the test:
0. install [eldev](https://github.com/doublep/eldev#installation)
1. `eldev compile`
2. `eldev test`
jxa added 2 commits August 9, 2021 22:34
Because the only autoloaded function is sclang-mode, the sclang file was
not being evaluated. This made it impossible to use-package properly in
order to configure the package. Combining the files makes the most sense
as sclang.el wasn't doing much anyways.
Copy link
Author

@jxa jxa left a comment

Choose a reason for hiding this comment

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

A lot has changed, so you may want to review commit-by-commit. I tried to include good commit messages as I went.

I have tested this latest version in my personal config on mac os (doom config), a vanilla emacs install on mac os (straight.el config), and a fresh debian vm (compiled from supercollider source with this version in the editors/sc-el directory)

.github/workflows/test.yml Show resolved Hide resolved
el/Eldev Show resolved Hide resolved
@@ -13,13 +13,8 @@
;; Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
;; USA


(eval-when-compile
Copy link
Author

Choose a reason for hiding this comment

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

Removed all these eval-when-compile calls because we need to require at runtime when autoloading

el/sclang-help.el Show resolved Hide resolved
el/sclang.el Outdated Show resolved Hide resolved
el/test/sclang-mode-test.el Show resolved Hide resolved
@dyfer
Copy link
Member

dyfer commented Aug 11, 2021

Thank you for your work on this and for detailed descriptions. I hope to get to this in the next few days, though it might take time as I don't know much about various aspects of scel. Your descriptions should make it easier though. GitHub Actions is definitely a nice "touch", thanks for adding that.

@jxa
Copy link
Author

jxa commented Aug 11, 2021

it might take time as I don't know much about various aspects of scel

No problem @dyfer! If you would like a pair to help with installation and usage we could set up a time.

Copy link
Member

@dyfer dyfer left a comment

Choose a reason for hiding this comment

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

Thanks!
Here's a "first round" of comments from trying your PR on macOS with Aquamacs. The issue with duplicate class names with SC-IDE is the biggest issue, and probably a change to fix it (renaming a folder) needs to be carefully checked for regressions with the CMake installation mode. More below.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
el/Eldev Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@rukano
Copy link

rukano commented Aug 16, 2021

Hi, I finally got to test this, and got my emacs-macs with doom running with SC! Thanks for this!
I'm just getting one error when I start the interpreter:

*** Welcome to SuperCollider 3.11.2. *** For help type C-c C-y.
ERROR: syntax error, unexpected '(', expecting NAME or CLASSNAME or WHILE or '['
  in interpreted text
  line 1 char 69:

  Emacs.lispPerformCommand('Documentsetproperty', [ 2, 'prSetTitle', #("*doom*" 0 6 (face nil)) ], false) 

But doesn't seem to affect the rest of the functionality :)

README.md Show resolved Hide resolved
@jxa
Copy link
Author

jxa commented Aug 16, 2021

ERROR: syntax error, unexpected '(', expecting NAME or CLASSNAME or WHILE or '['
  in interpreted text
  line 1 char 69:

  Emacs.lispPerformCommand('Documentsetproperty', [ 2, 'prSetTitle', #("*doom*" 0 6 (face nil)) ], false) 

@rukano thanks for trying it out! I'm curious exactly how to recreate that error. Can you share the relevant part of your doom config and what command you execute to cause that error?

@dyfer
Copy link
Member

dyfer commented Aug 16, 2021

Thanks @jxa
Couple more thoughts since I reviewed it:

  • I'm wondering whether it's maybe more elegant to keep the supercollider code in sc top-level directory...? If we were to do that, we could move the SC code into sc/scide_scel/, what do you think about that?
  • I'd still would like to see a sentence in the README like: "If you open a new buffer in emacs, run M-x sclang-mode to activate sclang mode" or something like that. Unless I'm missing something, this is not already present, correct?
  • This is a possibly larger question, but let's think about it for a second: if we install the quark, couldn't we use that repository instead of installing a package through straight-use-package or MELPA? Is that awkward in emacs ecosystem? Do I understand correctly that it could look like this:
// run in sclang
Quarks.install("....scel");
;; in emacs, add to the ~/.emacs file
(add-to-list 'load-path "~/Library/Application Support/SuperCollider/download-quarks/scel/el/") ;; check the path and provide examples for other OSs as well
(require 'sclang)

I don't want to de-rail the PR, it's already such an improvement, but this came to my mind recently and I wanted to get your opinion on it.

@jxa
Copy link
Author

jxa commented Aug 16, 2021

* I'm wondering whether it's maybe more elegant to keep the supercollider code in `sc` top-level directory...? If we were to do that, we could move the SC code into `sc/scide_scel`, what do you think about that?

I like it. I was a little sad to lose the /sc and /el symmetry :) I'll see if that works

* I'd still would like to see a sentence in the README like: "If you open a new buffer in emacs, run `M-x sclang-mode` to activate sclang mode" or something like that. Unless I'm missing something, this is not already present, correct?

sclang-mode is automatically activated when a .sc or .scd file is opened. Then all that is necessary is to call sclang-start (bound to C-c C-o).

One thing that bugs me: I would like to be able to autoload the sclang-start function (be able to call it before you (require 'sclang)). I haven't been able to get that working yet, so for now you either have to have a scd file or call require first.

* if we install the quark, couldn't we use that repository instead of installing a package through `straight-use-package` or MELPA? Is that awkward in emacs ecosystem? 

It's an excellent question. Something I hadn't considered. It would simplify the install instructions at least :)

It's a good idea and it wouldn't change anything for users who prefer to use straight, doom, etc. I was reviewing MELPA submission guidelines recently and I think it will take some more work to get this library to a place where they would list it. There are lots of compilation warnings, for example, which I haven't addressed because I felt it would be better to have a different PR for it.

So I'll rework the install instructions to remove reference to emacs package managers and have them add Quarks.folder to their load path and require from there. Sound good?

@dyfer
Copy link
Member

dyfer commented Aug 16, 2021

I like it. I was a little sad to lose the /sc and /el symmetry :) I'll see if that works

Great!

sclang-mode is automatically activated when a .sc or .scd file is opened. Then all that is necessary is to call sclang-start (bound to C-c C-o).

Yes, but what if you are starting with a new (empty) document? This is what I was doing when testing this, and it took me a moment to realize I need to activate sclang-mode. Or is the user supposed to open a new document in the sclang-mode from the get-go? Not sure I saw an option for that in Aquamacs's menu.

One thing that bugs me: I would like to be able to autoload the sclang-start function (be able to call it before you (require 'sclang)). I haven't been able to get that working yet, so for now you either have to have a scd file or call require first.

It sounds like a worthy addition, but I can't help/comment more on that with my limited knowledge of emacs/lisp, sorry.

It's a good idea and it wouldn't change anything for users who prefer to use straight, doom, etc. I was reviewing MELPA submission guidelines recently and I think it will take some more work to get this library to a place where they would list it. There are lots of compilation warnings, for example, which I haven't addressed because I felt it would be better to have a different PR for it.

Good to know, thanks for looking into that.

So I'll rework the install instructions to remove reference to emacs package managers and have them add Quarks.folder to their load path and require from there. Sound good?

Yes, that sounds great! Streamlining the installation instructions is definitely a plus!

One more comment for the readme: I'd make a warning stand out, something like:

Whatever option you choose, make sure not to mix installation methods. In particular, do not install the quark if you already have the supercollider-emacs package or if you compiled SuperCollider with the -DSC_EL=ON option.

Because SC "breaks" (library compilation fails) with duplicated classes, IMO it is worthy to make this clear.

@jxa
Copy link
Author

jxa commented Aug 20, 2021

All the readme comments I think are now addressed. I also figured out how to autoload sclang-start and added instructions for that. Thanks for all the feedback! We're in a much better place as a result

Copy link
Member

@dyfer dyfer left a comment

Choose a reason for hiding this comment

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

Thanks so much! This is very close now.

One comment not included below is that if I open a new buffer in Aquamacs and run M-x start-sclang, it starts properly, but doesn't set the current buffer to sclang-mode. I guess this is expected? But I thought I'd ask.

Otherwise there are few minor comments/suggestions.

It'd still like to test installation from source on Linux before approving this.
I've tried installing on Ubuntu and it works. However, at first it didn't - it seems that Emacs would not load scel from /usr/local/share/emacs.... After copying these files to /usr/share/emacs... everything worked. I'm not sure if this is particular to my system (Ubuntu 20.04) or whether this is something to mention in the readme as well.

el/sclang-help.el Show resolved Hide resolved
el/sclang-help.el Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
jxa and others added 2 commits September 4, 2021 09:46
Also, autoload `sclang-customize`, as that should be able to be called
interactively regardless of whether sclang is started
@dyfer
Copy link
Member

dyfer commented Sep 4, 2021

I've made a minor change to the readme, but I somehow messed up the first commit, sorry! I'm merging this now. Thanks again for your work on this!

@dyfer dyfer merged commit d8296a9 into supercollider:main Sep 4, 2021
@dyfer
Copy link
Member

dyfer commented Oct 17, 2021

@jxa are you still planning to submit a PR in the SC repo to update the submodule?

@jxa
Copy link
Author

jxa commented Oct 19, 2021

Thanks for the reminder @dyfer I'll put one up now

@friskgit
Copy link

I was very happy to see that this work is going on. I have completely missed it. I will try it as soon as I can! One question though: is this mainly a new method for installation, or are there changes to the sclang emacs mode itself?

@jxa
Copy link
Author

jxa commented Nov 16, 2021

is this mainly a new method for installation, or are there changes to the sclang emacs mode itself?

Hi @friskgit, this PR focused on methods of installation only. Some changes were made to how the code is organized (to enable proper autoloading) and one custom variable was added.

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.

5 participants