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

Port build to dune #40

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Leonidas-from-XIV
Copy link
Member

This will make the build of the library a bit more future proof and easier for people to understand and contribute given dune is well-accepted.

@Leonidas-from-XIV Leonidas-from-XIV marked this pull request as ready for review June 29, 2022 09:18
@Leonidas-from-XIV
Copy link
Member Author

I had to create calendar and calendarlib because dune doesn't like having public names not match OPAM names but I feel this is probably alright in this context.

@pmetzger
Copy link
Member

pmetzger commented Jul 1, 2022

@Leonidas-from-XIV Before I did in, did you also fix the weird not-releasing-from-trunk issue for this repo? We should really make it follow normal expectations.

@Leonidas-from-XIV
Copy link
Member Author

No, I haven't looked at releasing yet, but I don't quite see what would prevent releasing from trunk? I tried this branch and it looks like when you install it you are getting the same API as the 2.04 release. It is a bit tricky to test but at least my surface-level testing worked, and so did the tests.

I also noticed that there is a 3.x branch with a dune port. Maybe the 3.x port can be made the default branch, to give it more visibility that work is progressing?

@pmetzger
Copy link
Member

pmetzger commented Sep 1, 2022

I recommend we simply pull the stuff from the branch onto the trunk and make this work the way every other thing works.

@Leonidas-from-XIV
Copy link
Member Author

Just had a quick look at the changes in the 3.x branch and yeah, that's probably reasonable.

@pmetzger
Copy link
Member

pmetzger commented Sep 6, 2022

Please do it then.

@Leonidas-from-XIV
Copy link
Member Author

As I don't have commit rights, I can only create a PR: #41

@pmetzger
Copy link
Member

pmetzger commented Sep 8, 2022

I've merged the PR to pull the 3.x branch to the main branch. If you could clean this up I'll merge after. Also, I can give you commit access if you would like.

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