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

expose MMCore_version variables in MMCore.h #533

Closed
wants to merge 1 commit into from

Conversation

tlambert03
Copy link
Contributor

The MMCore version info is only declared in MMCore.cpp, making it a bit harder to access in bindings like pymmcore. (you have to resort to manually declaring it which is error prone. this exposes those variables in the public header

@marktsuchida
Copy link
Member

Is the goal to make it easier to access the version at run time from Python? If so we might consider a variant of CMMCore::getVersionInfo() that returns 3 numbers instead of a string. That can be a static member function (or even a free function) so that CMMCore need not be instantiated. I think this is better than exposing integer constants; something like

static void CMMCore::getVersion(int& major, int& minor, int& patch);

(As you know the pymmcore version number has a manually incremented "packaging" version (5th element) that is specific to pymmcore; also the manually updated version is tested for match with getVersionInfo().)

@tlambert03
Copy link
Contributor Author

tlambert03 commented Dec 19, 2024

it's about compile time, rather than runtime. this would enable:

https://github.com/pymmcore-plus/pymmcore-nano/blob/46fa1769ec4bdedb251d62c042135ca3adc61205/src/_pymmcore_nano.cc#L244-L245

...which removes the need to manually increment packaging info and avoids any possibility of the two going out of sync (i.e. it doesn't depend on pytest to catch it).

I'm curious, what would be the argument against exposing the MMCore version in the header?

@tlambert03
Copy link
Contributor Author

tlambert03 commented Dec 20, 2024

ah, I suppose a free function like this could work?

// In the header file
inline std::tuple<int, int, int> GetMMCoreVersion() {
    return {1, 2, 3}; // Replace with actual version constants
}

or not an inline, with an implementation in MMCore.cpp?

std::tuple<int, int, int> GetMMCoreVersion();

and then a binding could do this?

NB_MODULE(_pymmcore_nano, m) {
    m.attr("MMCore_version_info") = GetMMCoreVersion();
}

is that correct?

@marktsuchida
Copy link
Member

Yes, that was the idea. With my suggestion of CMMCore::getVersion() you can do it like this using an immediately invoked lambda (still no need to instantiate CMMCore):

m.attr("MMCore_version") = []() {
    int major, minor, patch;
    CMMCore::getVersion(major, minor, patch);
    return std::tuple{major, minor, patch};
}();

The only reason to do it with these ugly output parameters (int&) is because it matches the existing style and should also automatically work in MMCoreJ. Maybe if std::tuple (or something similar, like std::array<int, 3>) is found to "just work" with SWIG-Java we could go with the version that returns the triple -- but note that it still needs to work with SWIG 3 for Java, at least for now.

I don't want to put the MMCore version in the header, because then it won't return the version of the MMCore being run, but the version of the MMCore against which the code was compiled. These are the same thing in pymmcore[-nano], but could differ when using from either C++ or Java. One could argue that we should have access to both (build and run-time) versions, but the need for that has not arisen so far.

@tlambert03
Copy link
Contributor Author

I don't want to put the MMCore version in the header, because then it won't return the version of the MMCore being run, but the version of the MMCore against which the code was compiled

sounds good! that's reason enough for me :)

@tlambert03 tlambert03 closed this Jan 17, 2025
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