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

C-language API permits weaker backward compatibility requirements. #1798

Open
wants to merge 1 commit into
base: trunk
from

Conversation

@jhwoodyatt
Copy link

jhwoodyatt commented May 24, 2018

Change to the C-language API extracted from #1003 for introducing the CAML_API_VERSION mechanism to improve control of the backward compatibility requirement.

  • Add CAML_API_VERSION parameter in the "caml/compatibility.h" header.
  • Default value is 100.
  • Explicitly define CAML_NAME_SPACE if CAML_API_VERSION >= 400.
  • If CAML_API_VERSION < 403, then also #define caml_stat_heap_xxx_size.
  • If CAML_API_VERSION < 400, then also #define Modify, Begin_roots, End_roots.
  • Always include "caml/compatibility.h" from "caml/memory.h" header.
  • Never include "caml/compatibility.h" from other headers.
@jhwoodyatt jhwoodyatt force-pushed the jhwoodyatt:x-jhwoodyatt-api-version branch from 4384918 to 6fa8189 May 24, 2018
@damiendoligez damiendoligez self-assigned this May 31, 2018
@dra27 dra27 changed the base branch from 4.07 to trunk Jun 30, 2018
@dra27 dra27 changed the base branch from trunk to 4.07 Jun 30, 2018
@dra27

This comment has been minimized.

Copy link
Contributor

dra27 commented Jun 30, 2018

Sorry, changing the base did not do what I'd hoped - please could you rebase this commit on to trunk?

Copy link
Member

damiendoligez left a comment

There is a problem in compatibility.h. The rest looks OK.

You also need to rebase to trunk. (and also change the base? I'm not sure)

runtime/caml/compatibility.h Outdated Show resolved Hide resolved
@@ -465,6 +464,8 @@ CAMLextern struct caml__roots_block *caml_local_roots; /* defined in roots.c */
caml_modify (&Field ((block), caml__temp_offset), caml__temp_val); \
}while(0)

#if CAML_API_VERSION < 400

/*

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Jul 3, 2018

Contributor

I'm afraid that Begin_roots and End_roots are used in code generated by the venerable CamlIDL tool (https://github.com/xavierleroy/camlidl/). The tool hasn't been updated in a long time but still has users. Forcing them to set CAML_API_VERSION to 312 or something is not nice.

This comment has been minimized.

Copy link
@jhwoodyatt

jhwoodyatt Jul 5, 2018

Author

Okay. Now that this is on trunk and not 4.07 I'll get around to this after my vacation.

This comment has been minimized.

Copy link
@jhwoodyatt

jhwoodyatt Jul 26, 2018

Author

So, first question about this: if the venerable CamlIDL doesn't set CAML_API_VERSION then it defaults to 100, and Begin_roots/End_roots definitions are available accordingly. Am I mistaken?

- Add CAML_API_VERSION parameter in the "caml/compatibility.h" header.
- Default value is 100.
- Explicitly define CAML_NAME_SPACE if CAML_API_VERSION >= 400.
- If CAML_API_VERSION < 403, then also #define caml_stat_heap_xxx_size.
- If CAML_API_VERSION < 400, then also #define Modify, Begin_roots, End_roots.
- Always include "caml/compatibility.h" from "caml/memory.h" header.
- Never include "caml/compatibility.h" from other headers.
@jhwoodyatt jhwoodyatt force-pushed the jhwoodyatt:x-jhwoodyatt-api-version branch from 6fa8189 to 2f0769e Jul 26, 2018
@jhwoodyatt jhwoodyatt changed the base branch from 4.07 to trunk Jul 26, 2018
@jhwoodyatt

This comment has been minimized.

Copy link
Author

jhwoodyatt commented Jul 26, 2018

I have rebased this on trunk and addressed the "fishy" thing that @damiendoligez noticed. I'm ready to make whatever change @xavierleroy thinks is necessary to keep CamlIDL from being damaged, but I'm not sure I know what to do there.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

xavierleroy commented Jul 26, 2018

My problem is that I'm not sure what CAML_API_VERSION=100 means:

  1. all runtime APIs since version 1.00 can be used, or
  2. only runtime APIs that were in version 1.00 can be used.

CamlIDL is a case where we need both old APIs (for code generated by CamlIDL) and newer APIs (for code hand-written by users to do things that CamlIDL cannot handle).

@jhwoodyatt

This comment has been minimized.

Copy link
Author

jhwoodyatt commented Jul 26, 2018

My problem is that I'm not sure what CAML_API_VERSION=100 means...

I had this problem too, and I agree it's confusing. Here's a proposal, which I hope perhaps @stedolan or somebody else more central to the multicore effort will review, that would expand the set of configuration variables used for API backward compatibility selection.

  1. Change all the instances of CAML_API_VERSION in this change to CAML_API_COMPATIBILITY.
  2. Introduce another configuration variable CAML_API_VERSION which matches the version in the top level VERSION file. (I might be able to write some logic that automatically generates the definition during the compiler build.)
#define CAML_API_COMPATIBILITY 100
#define CAML_API_VERSION 408

This would allow consumers of the runtime API A) to assert their minimum API compatibility using definitions of the CAML_API_COMPATIBILITY variable before inclusion of <caml/compatibility.h>, and B) to adapt their internal logic to version of the API in the current compiler switch.

@jhwoodyatt

This comment has been minimized.

Copy link
Author

jhwoodyatt commented Sep 10, 2018

Gentle acknowledgement that I'm still holding the ball on this problem, and I'm not sure what to do with it.

@damiendoligez

This comment has been minimized.

Copy link
Member

damiendoligez commented Nov 16, 2018

If I understand correctly, your CAML_API_COMPATIBILITY variable is defined by the user and is the minimum API version needed by the user's code, while the CAML_API_VERSION is the actual version number and defined by the runtime's headers.

@jhwoodyatt

This comment has been minimized.

Copy link
Author

jhwoodyatt commented Nov 27, 2018

If I understand correctly, your CAML_API_COMPATIBILITY variable is defined by the user and is the minimum API version needed by the user's code, while the CAML_API_VERSION is the actual version number and defined by the runtime's headers.

That's what I think I was doing, yes. Again, I'm not sure this pull request is really the appropriate way to address the underlying problem, or even if the underlying problem is important enough to address. My interest in it developed when I tried to make some of my personal code with a few C-language foreign functions in it compile under the Multicore OCaml branch, which seems like it will need something in the runtime interface that allows extension implementations to compile according to the version of the runtime available.

@jhwoodyatt

This comment has been minimized.

Copy link
Author

jhwoodyatt commented Nov 27, 2018

When I get a few spare cycles next, I'll see about resolving those merge conflicts.

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Sep 20, 2019

@johnwhitington will be this part of the 4.10 by any chance?

@johnwhitington

This comment has been minimized.

Copy link
Contributor

johnwhitington commented Sep 20, 2019

@XVilka I assume that was autocomplete for @jhwoodyatt :-)

@XVilka

This comment has been minimized.

Copy link
Contributor

XVilka commented Sep 20, 2019

Yes, sorry about that.

@gadmm

This comment has been minimized.

Copy link
Contributor

gadmm commented Dec 7, 2019

This is superseded by #9167 in case it is accepted. Please see the discussion there.

@jhwoodyatt

This comment has been minimized.

Copy link
Author

jhwoodyatt commented Dec 10, 2019

I'm tracking the progress of #9167 and will withdraw this if that achieves consensus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.