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

Some clean up in CONTRIBUTING.md and fix broken links & redirects #1038

Merged
merged 9 commits into from
Nov 8, 2024

Conversation

arvidj
Copy link

@arvidj arvidj commented Nov 1, 2024

This PR:

  • updates the recommendation for how to set up a switch for lwt to recommend ocaml 4.08 (lowest supported OCaml version IIUC)
  • removes mentions of appveyor and travis-ci as these are replaced by github actions
  • fixes broken links and redirects, and reference-style markdown links that were not used

Arvid Jakobsson added 8 commits November 1, 2024 09:55
@arvidj arvidj force-pushed the arvid@clean-up-contributing branch from b46ba85 to 59006a9 Compare November 1, 2024 15:09
@arvidj
Copy link
Author

arvidj commented Nov 1, 2024

Fixes #839

Comment on lines -14 to -26
(* Check if this is running inside Travis or AppVeyor. *)
let in_travis =
try ignore (Sys.getenv "TRAVIS_COMMIT"); true
with Not_found -> false
in

let in_appveyor =
try ignore (Sys.getenv "APPVEYOR_REPO_COMMIT"); true
with Not_found -> false
in

if not (in_travis || in_appveyor) then Lwt.return_true
else Lwt.return Lwt_config.libev_default);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of deleting this, you could check whether the CI env var is true or True, or check whether GITHUB_ACTIONS is true.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't quite sure of the meaning of this test. Since in_travis and in_appveyor would always be false, it's basically a noop. I can do what you propose, and then the meaning of the test would be "if we're in github actions, then check that Lwt_config.libev_default is true". Is that what we want the test to check (if so, why do we want to test that?)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably left-over from special-casing for different CIs and I think we can remove it.

- drop reddit info (deprecated)
- add reference to lwt_domain
@raphael-proust
Copy link
Collaborator

Thanks a lot for the long overdue fixes

@raphael-proust raphael-proust merged commit 347c3ad into ocsigen:master Nov 8, 2024
26 of 28 checks passed
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