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

mlgmpidl and mlmpfr defines the same module #21

Closed
bobot opened this issue May 25, 2022 · 7 comments
Closed

mlgmpidl and mlmpfr defines the same module #21

bobot opened this issue May 25, 2022 · 7 comments

Comments

@bobot
Copy link
Contributor

bobot commented May 25, 2022

The module mpfr is defined in both libraries, so we can't link both of them at the same time (e.g Apron and Why3). It is possible to prefix one of them and still provide a backward compatible version that automatically make the translation. But @nberth and @thvnx are you willing to fix this incompatibility?

@thvnx
Copy link
Owner

thvnx commented May 25, 2022

I don't mind prefixing mine, I could probably use mlmpfr instead of mpfr. WDYT?
About the backward compatibility, is that possible?, any idea on how to achieve it?

@nberth
Copy link

nberth commented May 25, 2022

Indeed that's a bit of an issue.

It is possible to prefix one of them and still provide a backward compatible version that automatically make the translation.

Do you mean that users of one of the packages would explicitly rename the Mpfr module by default, and packages that use it would need to be adapted to use the renamed version?
Then a separate package name would need to be used to explicitly retain backward compatibility.

@bobot
Copy link
Contributor Author

bobot commented May 25, 2022

Simple but not perfect

Dune as this feature:

(wrapped (transition )) is the same as (wrapped true), except it will also generate unwrapped (not prefixed by the library name) modules to preserve compatibility. This is useful for libraries that would like to transition from (wrapped false) to (wrapped true) without breaking compatibility for users. The deprecation notices for the unwrapped modules will include .

If it is used for mlmpfr, it makes the module Mpfr be

[@@@deprecated "<message>. Use Mlmpfr.Mpfr instead."] include Mlmpfr__Mpfr

So you get a deprecation message by the compiler if you use Mpfr to use Mlmpfr.Mpfr instead. It works well for static linking because if someone doesn't use Mpfr it is not linked. However when using dynamic linking or static linking with --linkall, the Mpfr module is still linked and so the clash could occur.

Solution that handle dynamic linking

Perhaps the best would be to use a separate library:

  • step 1:

    • library mlmpfr.wrapped with all the code and wrapped
    • library mlmpfr with deprecation message, and Mpfr includes mlmpfr_wrapped__Mpfr
  • step 2 (n year later?):

    • library mlmpfr.wrapped with deprecation message and includes Mlmpfr
    • library mlmpfr with all the code and wrapped
  • step 3 (n year later?):

    • library mlmpfr with all the code and wrapped

Advantage step 1, backward compatible and allow people to use a fully wrapped Mpfr safe version. step 2 and step 3, backward compatible just with the previous step.

@thvnx
Copy link
Owner

thvnx commented May 30, 2022

Good to know! I thought about it this weekend and I think I'll just simply rename the module, breaking the backward compatibility. As far as I know, you're the only active user of mlmpfr, so I can afford such a change for now. I'll prepare a PR and a new opam release in the next few days.

@nberth
Copy link

nberth commented May 30, 2022

Cool, thanks a lot @thvnx if you can do this. mlgmpidl already has its fair share of users, and in addition it does not use dune as build system so the renaming transition would have probably been quite painfull 😅

thvnx added a commit that referenced this issue May 30, 2022
This change breaks backward compatibility.

See issue #21
thvnx added a commit that referenced this issue May 30, 2022
This change breaks backward compatibility.

See issue #21
thvnx added a commit to thvnx/opam-repository that referenced this issue May 30, 2022
This patch remanes the main mlmpfr module from Mpfr to Mlmpfr to avoid
name clashes with the mlgmpidl's mpfr module.

Warning: it breaks the backward compatibility.

See also thvnx/mlmpfr#21.
@thvnx
Copy link
Owner

thvnx commented May 31, 2022

Should be good now. @bobot, there is a new release on opam that you can use for your next release of wh3 ;).

@thvnx thvnx closed this as completed May 31, 2022
@bobot
Copy link
Contributor Author

bobot commented Jun 1, 2022

Thank you, I hope we will be able to handle the transition without hurt.

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

No branches or pull requests

3 participants