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

Add patches files to opam #10507

Closed
wants to merge 1 commit into from
Closed

Add patches files to opam #10507

wants to merge 1 commit into from

Conversation

Nyavlys
Copy link
Contributor

@Nyavlys Nyavlys commented Oct 20, 2017

I added patches files in files/ In order to have acgtk.1.3.1 compile with easy-format:version >1.2.0 and yojson:version>1.3.3 as well.

@camelus
Copy link
Contributor

camelus commented Oct 20, 2017

❗ opam-lint warnings af52cc9
  • acgtk.1.3.1 has some warnings:

    • warning 37: Missing field 'dev-repo'

✅ Installability check (7603 → 7603)

@gasche
Copy link
Member

gasche commented Oct 28, 2017

Where are the patches coming from? Did you write them yourself? Have them been submitted in the package upstream? Merged? Is patching better than doing a new release?

@dbuenzli
Copy link
Collaborator

Also this #10531 seems to be of interest in this case.

@Nyavlys
Copy link
Contributor Author

Nyavlys commented Oct 30, 2017

Hi,

Yes, I wrote the patches by myself. I chose to patch rather than to make a new release because the code of the package itself is unchanged.

The patches only concern makefiles (and configuration files) to take into account the fact that some packages my package depends on have changed they way the have to be linked.

On the other hand, if it makes life easier, I can release a new version with that modifications only, I don't really care.

Regarding the check failures (in case it helps):

  • I noticed that patching fails with opam V2.0 and CentOS but succeeds with any other version/distribution combination (including 1.2/CentOS)

  • there is still the same old problem with MacOS and bolt, or more precisely some camlp4 file that is not installed in travis/MacOS checks (bolt.1.4 build failure on OS X #7436)

Let me know if you prefer a new release instead of patches.

@gasche
Copy link
Member

gasche commented Oct 30, 2017

This does make sense to me, thanks -- although I would recommend that you submit them upstream for inclusion, to avoid divergence of the opam-packaged and upstream codebase. @dbuenzli, what do you think?

@dbuenzli
Copy link
Collaborator

Yes. I think it's better not to have patches in the repo.

@Nyavlys
Copy link
Contributor Author

Nyavlys commented Oct 30, 2017

Ok. I'll make a new release, then.

@dbuenzli
Copy link
Collaborator

Thanks @Nyavlys this is very much appreciated.

@gasche gasche closed this Oct 30, 2017
@Nyavlys Nyavlys mentioned this pull request Nov 28, 2017
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.

4 participants