-
Notifications
You must be signed in to change notification settings - Fork 1
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
WIP: try out the autocxx branch #11
Conversation
This is the error I'm getting now.
|
Seems like all the C++ errors are gone now. Now I'm getting these from Rust:
|
Getting close. Now it's a linker error. 🤪
|
crates/mfem-sys/build.rs
Outdated
} | ||
if let "windows" = std::env::consts::OS { | ||
let current = std::env::current_dir().unwrap(); | ||
build.include(current.parent().unwrap()); | ||
} | ||
build.define("MFEM_USE_DOUBLE", Some("true")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: None
would also work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it does the right thing. The MFEM library should report whether it was compiled with single or double precision — you cannot change that in mfem-sys
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I added this line just to get it working.
When mfem-cpp
has the bundled
feature enabled, this setting must also be provided, correct?
What do you think about introducing a feature for this, e.g. single-precision
(making double precision the default)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear: it doesn't need to happen in this change. We can just file an issue and do it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed some additional commits to the autocxx branch. It is better to do it know—it affects the code generation (need to use real_t
). Moreover, it pushed me to better understand the mfem-sys
cmake call. I also had some problem with defining a type to specify cxx
functions that may be f32
or f64
depending on the library. Maybe it is possible to do better.
Are you using the latest version of the branch — this is normally fixed in later commits. |
Sorry, I don' have much time to work on this these days — I'm full with other duties... I will try to set up CI so tests on various platforms ans various compiler versions are automatically performed. |
I set up CI. Linux and MacOS pass (with the |
e51bba4
to
b6e25b9
Compare
I've rebased this branch onto your latest |
I've rebased this branch onto your latest autocxx branch.
Still getting that linker error.
There are more commits on my repo but I'd like the Windows CI to pass before pushing them.
Might be related to the recent upgrade to macOS 14.4... 🤔
Well, at least you have access to the library and can list the symbols. Less fun on the CI! 😅
|
b6e25b9
to
ad2af86
Compare
@Chris00 I've incorporated your CI config into ...but it fails with a runtime error:
|
I cannot reproduce it:
but this is interesting: some memory was maybe freed twice? |
@Chris00 You're right; I just tried it on another computer (a Mac Mini M2), and getting your results now. So something is off on the Macbook. I'll try to re-clone the repo... |
@Chris00 Yep, after re-cloning the repo, I'm getting the same results as you. I'd like to rebase the |
Please see #8 (comment) |
May you try the current |
The environment variable DEP_MFEM_ROOT has been renamed MFEM_DIR as this seems to be the expected name https://mfem.org/building/
When mfem-cpp has the bundled feature enabled, this setting must also
be provided, correct?
No, the precision with which mfem was compiled is provided in config/_config.hpp — we should not override that. That file actually provides more macros giving information about mfem compilation (such as MFEM_INSTALL_DIR). The information about single/double precision is also accessible via cmake. ATM in the autocxx branch, mfem-sys/build.rs uses that to check that mfem was compiled with double precision and issues an error otherwise (mfem-cpp specifically requests double precision—I added a `define` for that a few commits back).
What do you think about introducing a feature for this,
e.g. single-precision (making double precision the default)?
In such case, libraries depending on mfem will need to define their feature to reflect the choice of mfem and write conditional code. However, the choice of precision for mfem only makes sense for `bundled`; if the library was installed system-wide, the compilation must fail in case the precision mismatches. What should the feature name be? `precision-f32`?
Note that the mfem `real_t` is already accessible from the autocxx bindings — we have to find a better name for the “Rust like” bindings.
|
0bc2c77
to
d67ce58
Compare
@Chris00 I'm trying to make the ![]() EDIT: I just needed to |
d67ce58
to
2ea7c26
Compare
2ea7c26
to
ef6142a
Compare
You are right, it points to the wrong commit. I don't know how it is possible that the CI tests did not fail (I cloned this repository afresh and |
Just an idea, but we could ask them to create a |
Good idea. I've done so. |
@Chris00 This is ready, and all checks are passing. Are you okay with merging this into the |
Trying to make it work on my setup.
Rust 1.84.1 on macOS, Macbook Pro M2.