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

changes for MELPA compatibility #46

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

zzkt
Copy link

@zzkt zzkt commented Aug 2, 2022

These changes should provide a reasonable basis for adding sclang to MELPA in line with their PR requirements.

The changes are mostly superficial as suggested by checkdoc, lint-package and eldev. They do not add any new functionality and include things like new docstrings for functions, replacing obsolete stuff, formatting changes, and so on.

There are a few things to note...

  • including w3m as a dependency for the help browser will raise the minimum required emacs version to 27.1
  • the regex linter from eldev gives a few warnings for sclang-language.el(details at fix regexps zzkt/scel#2) so probably doesn't work as expected and fully automated linting for CI isn't really useful at the moment.
  • some of the docstrings might not be accurate or need further clarification (but shouldn't block things)
  • in some places the existing code probably doesn't do what is expected (which could block things)
  • i've generally left existing code unchanged, so will add new issues before making any more substantial changes.
  • bumped version to sclang 1.1.0

etc

@zzkt
Copy link
Author

zzkt commented Aug 16, 2022

Is there someone who would be interested in reviewing this PR? or can you let me know if there is more/other work needed? @dyfer or @jxa?

@dyfer
Copy link
Member

dyfer commented Aug 16, 2022

Thanks for the ping @zzkt, I missed this PR.

@jxa would you be interested in reviewing this, especially from the emacs perspective?

@zzkt, have you tested whether cmake installation method still works after these changes? That would be one thing I'd be looking out for when reviewing this.

Also, how drastic of a change is to require emacs 27.1? And does that requirement still stands if scel is installed as a quark or from cmake?

@zzkt
Copy link
Author

zzkt commented Aug 17, 2022

Thanks @dyfer I've checked building with cmake and it should work as expected. In 5a5ff2b I've added changes to include locally installed emacs packages (i.e. w3m) if they are in the standard paths (this could also be a build option?)

Since w3m is required for viewing the help files it needs to be installed or emacs will return an error during build. It might be helpful to add a more verbose message with cmake.

Adding Package-Requires: ((emacs "27.1") (w3m "0.0")) should only concern installing as a package, so in theory if built via cmake it won't cause problems. However, there could be runtime issues trying to use w3m on emacs < 27.1

@zzkt
Copy link
Author

zzkt commented Aug 17, 2022

There are some minor changes in the scel CI workflow which checks the package build and includes linting (and potentially more strict checks at some point) but no cmake.

I assume there is something in https://github.com/supercollider/supercollider that would pick up any other build errors?

@dyfer
Copy link
Member

dyfer commented Aug 17, 2022

Thanks for your work and again sorry for lack of my understanding here.

Since w3m is required for viewing the help files it needs to be installed or emacs will return an error during build.

With this PR we'd have 4 ways to install scel: as SC quark, from MELPA, from debian package and when building SC from source. Would the lack of w3m installed cause error in all these methods? Does this PR change the behavior for installing this as a quark or from source?

I assume there is something in https://github.com/supercollider/supercollider that would pick up any other build errors?

Yes, hopefully...

You can check our CI relatively easily: 1. fork supercollider, 2. create a new branch based on develop, 3. change scel submodule to use your scel fork, 4. push the SC branch to your SC fork. The CI will run and you can see if anything comes up.

Relatedly - if/when this PR gets merged, we should remember to follow up with updating supercollider submodule.

@jxa
Copy link

jxa commented Aug 18, 2022

Hey @zzkt I should have some time in the next couple of days to pull this branch and take a closer look.

@zzkt
Copy link
Author

zzkt commented Aug 18, 2022

At the moment w3m is not an explicit dependency during the build or install only because it's commented out (probably to avoid build errors?)

;; (require 'w3m) ;; not needed during compilation

I think it makes sense to either install or check for it during the build (or make it optional when installing scel from source which needs more work...)

looks like a few things would need updating...

  • MELPA package adds w3m as a requirement and it's handled by the package manager
  • check or prompt during the cmake install when the emacs ide is a build option (i'm not sure of the specifics)
  • on debian the supercollider-emacs package could depend on w3m-el (at the moment it only Recommends: w3m-el)
  • not sure what install via quark needs (but looks like it's also assuming w3m is installed)

@zzkt
Copy link
Author

zzkt commented Aug 18, 2022

There are still some issues with the supercollider CI build. I'm looking into it (some details at zzkt#3)

@zzkt
Copy link
Author

zzkt commented Aug 19, 2022

Looks like the cmake build is working as expected for supercollider and scel.

I've added some kludgy stuff (seen here) to check if w3m is installed locally and there also needs to be a small addition to the supercollider CI zzkt/supercollider@6711d20 (and possibly elsewhere?)

@zzkt
Copy link
Author

zzkt commented Sep 13, 2022

have you had a chance to check the changes jxa? anything else I should be looking at?

@zzkt
Copy link
Author

zzkt commented Oct 31, 2022

blip @dyfer and/or @jxa

@dyfer
Copy link
Member

dyfer commented Oct 31, 2022

@jxa do you have any more comments for this?

@dyfer
Copy link
Member

dyfer commented Jan 16, 2023

@jxa would you be able to take a look at these changes?

@zzkt
Copy link
Author

zzkt commented Feb 3, 2023

@dyfer and @jxa another option could be that i link to another repo for the melpa packages and merge from there as required?

@zzkt
Copy link
Author

zzkt commented Jun 6, 2023

re.blip @dyfer and/or @jxa

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.

3 participants