-
Notifications
You must be signed in to change notification settings - Fork 59
Remove -j-std
#425
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
Remove -j-std
#425
Conversation
|
|
Building the repo with |
mjambon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you!
|
@mjambon should we do anything special with PRs that target the "3.0" major version (ie breaking changes we may not want in a minor release)? |
|
@smondet sorry I forgot to follow up. We should edit ocaml-ci fails because it uses the latest yojson (3) which is allowed by the current version constraint. I'm not sure why our own CI succeeds. I guess we should also make it use yojson 3 and not bother supporting yojson 2 any longer (except for security patches if ever needed). @kit-ty-kate is asking for a yojson3-compatible release at #431 |
@mjambon That does not fix the issue for dependents, unfortunately, since two versions of Which won’t work, of course. |
|
I didn't look the changes in details, but could |
As far as i understand the code, i believe this is what this PR does already. The option is kept and doesn't do anything unless |
|
I'm going to merge this. The tests pass with yojson 2.0.2. They still fail with yojson 3.0.0 but this isn't new and there's a version constraint in the relevant opam file ensuring the use of yojson 2. I'll try to fix the compatibility with yojson 3.0.0 in another PR. |
8340908 to
bd4916f
Compare
Cf. issue #412
PR checklist
CHANGES.mdis up-to-date