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

Sideloader: handle binary files, support multiple directories #3041

Merged
merged 3 commits into from
Sep 2, 2021

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Sep 1, 2021

Make sure that files are served byte-for-byte as they are on disk, without emacs
doing any coding system conversion. This also prevents base64-encode from
complaining about multibyte characters.

Support multiple directories as sources for the sideloader, e.g.
cider-nrepl/src, cider-nrepl/resources, orchard/src

Part of #3037

Discovered this because cider.nrepl.middleware.track-state contains a U+2019 RIGHT SINGLE QUOTATION MARK :)


  • The commits are consistent with our contribution guidelines
  • You've added tests (if possible) to cover your change(s)
  • All tests are passing (eldev test)
  • All code passes the linter (eldev lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

@plexus
Copy link
Contributor Author

plexus commented Sep 1, 2021

Oops there's actually another important change in there, I should split that up. (serving from multiple directories)

@plexus plexus changed the title Handle multibyte/binary data in sideloader, start tooling sideloader Sideloader: handle binary files, support multiple directories Sep 1, 2021
cider-eval.el Outdated
(defvar cider-sideloader-dir (file-name-directory load-file-name))
(defvar cider-sideloader-dirs
(list (file-name-directory load-file-name))
"Directories where we look for resources requested by the sideloader")
Copy link
Member

Choose a reason for hiding this comment

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

I think the missing . is breaking the CI.

@bbatsov
Copy link
Member

bbatsov commented Sep 1, 2021

Discovered this because cider.nrepl.middleware.track-state contains a U+2019 RIGHT SINGLE QUOTATION MARK :)

Wow! 🤣

Support multiple directories as sources for the sideloader, e.g., cider-nrepl/src, cider-nrepl/resources, orchard/src

Seems you've address my comment from the meta issue already. :D Probably we should make this eventually a defcustom to which we always append CIDER's source dir + lib (or something along those lines).

Seems the win build has hit some CircleCI limit... No love for OSS. :-)

@plexus
Copy link
Contributor Author

plexus commented Sep 2, 2021

That is some funny math going on at CircleCI

2021-09-02_150545_

Make sure that files are served byte-for-byte as they are on disk, without emacs
doing any coding system conversion. This also prevents base64-encode from
complaining about multibyte characters.

Support multiple directories as sources for the sideloader, e.g.
cider-nrepl/src, cider-nrepl/resources, orchard/src
@plexus
Copy link
Contributor Author

plexus commented Sep 2, 2021

The linting is fixed. The windows builds are still red but that just seems to be a CircleCI limit we've hit.

@bbatsov
Copy link
Member

bbatsov commented Sep 2, 2021

That's pretty weird, as supposedly we should get a pretty high limit on an OSS plan https://circleci.com/open-source/ I guess I'll have to contact their support (if they provide any for the OSS projects).

@bbatsov bbatsov merged commit 053562e into clojure-emacs:master Sep 2, 2021
@plexus
Copy link
Contributor Author

plexus commented Sep 7, 2021

My contact at CircleCI says the limits for Windows builds are different since they're more expensive. Seems we should use them sparingly.

  • Do we need to lint separately on Windows?
  • Maybe we can convert the windows jobs to GH actions?

@plexus plexus deleted the sideloader-improvements branch September 7, 2021 11:28
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.

2 participants