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

Fix sesman-restart regression issue with SIGHUP handling in server #3363

Merged
merged 2 commits into from
Jul 2, 2023

Conversation

ikappaki
Copy link
Contributor

@ikappaki ikappaki commented Jul 1, 2023

Hi,

can you please consider fix for sesman-restart regression issue. Fixes #3362.

It basically restores handling of sighup to close the connections, and add a test to confirm its working.

The mock server has been updated to respond the eval and close op codes. I've re-jiggled a bit with the log functions in there to make them more useful.

Thanks

cc @chopptimus


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • 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)

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

@ikappaki
Copy link
Contributor Author

ikappaki commented Jul 1, 2023

Btw, I've noticed that the integration tests are frequently failing in the win and macos builds, and always has to do with networking or bootstrapping issues rather than actual test issues.

For example, currently the win 27.2 build has failed while bootstraping eldev for Emacs.

Bootstrapping Eldev for Emacs 27.2 from MELPA Stable...

[00:03.758] End of file during parsing
[00:03.758] When updating contents of package archive `melpa-unstable'
Error: Process completed with exit code 1.

I don't think there is an easy solution for what I consider to be GH runner or network connections issues. Shall we just reduce the version range just to the latest Emacs version (28.2) for macos and win? This will I think reduce the likelihood of the whole integration test run failing, while there will still be the older supported version tested with the ubuntu builds.

image

Thanks

@vemv
Copy link
Member

vemv commented Jul 1, 2023

@ikappaki
Copy link
Contributor Author

ikappaki commented Jul 1, 2023

I'd be OK with retries in CI steps, WDYT?

e.g. https://github.com/clojure-emacs/enrich-classpath/blob/7bd8b585a20fa7818ce7a6f30d9c39e02228cbc8/.circleci/config.yml#L137

Personally I think we are better of reducing the windows/macos builds, we don't get much value out of them given that we also test the same on ubuntu, and the chances that there is a regression in the older maxos/win versions that don't appear in the ubuntu peers is slim.

Nevertheless I'll give it a try.

@ikappaki
Copy link
Contributor Author

ikappaki commented Jul 1, 2023

I'd be OK with retries in CI steps, WDYT?
e.g. https://github.com/clojure-emacs/enrich-classpath/blob/7bd8b585a20fa7818ce7a6f30d9c39e02228cbc8/.circleci/config.yml#L137

Personally I think we are better of reducing the windows/macos builds, we don't get much value out of them given that we also test the same on ubuntu, and the chances that there is a regression in the older maxos/win versions that don't appear in the ubuntu peers is slim.

Nevertheless I'll give it a try.

I've updated test.yml to repeat the integration tests once more in case of failure. Let's see how this goes :)

test/cider-tests.el Show resolved Hide resolved
(mapc #'cider--close-connection clients)

;; see https://github.com/clojure-emacs/cider/pull/3333
(when (string-match-p "^hangup" event)
Copy link
Member

Choose a reason for hiding this comment

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

It'd be better to explain in comments this behavior, so people don't have to lookup the github issue, but this will do for now.

@bbatsov bbatsov merged commit c58eebe into clojure-emacs:master Jul 2, 2023
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.

sesman-restart error
3 participants