Skip to content

Conversation

jesse-greathouse
Copy link

omni-irc.v0.1.12-1-g01e9475

Omni IRC: meta package for the monorepo
Meta package for the Omni-IRC libraries. Install the specific subpackages: omni-irc-sig, omni-irc-conn, omni-irc-engine, omni-irc-io-{tcp,tls,unixsock}, omni-irc-ui, omni-irc-ui-notty (and optionally omni-irc-client).



🐫 Pull-request generated by opam-publish v2.5.1

@jmid
Copy link
Member

jmid commented Sep 10, 2025

Thanks!

The proposed package omni-irc has a number of dependencies:

  "omni-irc-sig"
  "omni-irc-conn"
  "omni-irc-engine"
  "omni-irc-io-tcp"
  "omni-irc-io-tls" {"os" != "win32"}
  "omni-irc-io-unixsock" {"os" != "win32"}
  "omni-irc-ui"
  "omni-irc-ui-notty" {"os" != "win32"}

None of these are included in the PR (or available on the opam-repo), meaning the package cannot be installed 🤔 Did you exclude these other omni- packages from the PR by mistake?

The Windows constraints such as "omni-irc-io-tls" {"os" != "win32"} means "omni-irc depends on omni-irc-io-tls", except on Windows. What that your intention?

Finally, with a bunch of packages living in the same repo, that's a recipe for cross-version-constraint problems (for you - and for the opam-repo maintainers). Here it is helpful to state lock-step requirements, e.g., omni-irc.0.1.12 requires omni-irc-sig.0.1.12, etc. This can be expressed with the special version variable:

  "omni-irc-sig" {= version}
  "omni-irc-conn" {= version}
  ...

Finally I can see you are versioning the release with a v prefix, which is against the current policy
https://github.com/ocaml/opam-repository/tree/master/governance/policies#6-version-numbers-should-not-have-a-v-prefix-not-v120-but-120
(I'm also a bit unsure of how version tooling will hold up to the g01e9475 suffix, but I'll let others comment on that)

@jesse-greathouse
Copy link
Author

The proposed package omni-irc has a number of dependencies:

I think there is a mistake in my understanding of how to position the repository.
All of the modules in the monorepo should have been included but they apparently weren't. That was a misunderstanding on my end, and I apologize for the confusion.

I was submitting via the opam command cli and I thought I was making it commit the whole repository which includes all of those other modules.

I will resubmit this monorepo as an all inclusive collection of modules because they are intended to be replaced or extended, but if they don't come all together they will not likely be useable as they will not fit into any other system.

Finally, with a bunch of packages living in the same repo, that's a recipe for cross-version-constraint problems (for you - and for the opam-repo maintainers). Here it is helpful to state lock-step requirements, e.g., omni-irc.0.1.12 requires omni-irc-sig.0.1.12, etc. This can be expressed with the special version variable:

  "omni-irc-sig" {= version}
  "omni-irc-conn" {= version}
  ...

Finally I can see you are versioning the release with a v prefix, which is against

I do a compliant use of versioning when I resubmit, sorry about that.

the current policy https://github.com/ocaml/opam-repository/tree/master/governance/policies#6-version-numbers-should-not-have-a-v-prefix-not-v120-but-120 (I'm also a bit unsure of how version tooling will hold up to the g01e9475 suffix, but I'll let others comment on that)

The Windows constraints such as "omni-irc-io-tls" {"os" != "win32"} means "omni-irc depends on omni-irc-io-tls", except on Windows. What that your intention?

In the beginning I thought that the TLS library would not work on windows, but after some compromise I am finding that the ocaml-tls functionality works just fine, it just requires a slight alteration to the TLS coupling surface that I designed, but pending the testing on windows is succeeding, I am going to change the Windows dependency surface to include TLS because now I can see that TLS will work.

@shonfeder
Copy link
Contributor

I do a compliant use of versioning when I resubmit, sorry about that.

No worries! Thanks for the effort to share your work here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants