-
Notifications
You must be signed in to change notification settings - Fork 30
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
number_type.h
it's still mentioned in morph/CMakeLists.txt
#336
Comments
Hi, thanks for the message. I can resolve this in main soon. Is that enough to ease your packaging efforts or do you need this change to make its way into a release? I also noticed you made a few small tweaks such as changing the way nlohmann json is handled. If you have any suggestions for best practice I can absorb in the code base I'd welcome them. Seb |
Thanks for your prompt answer ! A new release would be ideal, but I can work with a commit too by modifying the I would suggest adding some options in the |
I have one more question: are the fonts required only for the examples or are they needed in general ? |
I like the bundled dependencies simply because they avoid a programmer having to clone recursively. However, I don't think this is a sustainable long term solution, so your suggestion of an option is a good one. You're right that healpix is considerably modified, so that has to stay bundled. BUILD_TESTS and BUILD_EXAMPLES is a good call - examples take a long time to build fonts ARE required, because when you compile a morphologica program it compiles the fonts into the binary. This avoids the programs from needing the fonts to have been installed - they're completely portable: https://abrg-models.github.io/morphologica/ref/visualint/visualface |
I've made the various changes discussed and created a new release. Let me know how you get on. I'll leave this issue open for now, in case anything else comes up. |
Thanks ! I've modified the PR and I'm waiting for the pipeline to finish. |
That's great. Perhaps your vcpkg will make it possible/easy for me to create a Windows CI Github action? |
Do you want to create a Windows CI for the main branch, right ? About Windows, I had some issues when compiling morphologica on it. I received some errors about undefined function from |
I did compile the system for Windows some time ago, and put a lot of work into making it possible to build on Windows. I had to work on the process of including a binary in the executable, for which I used incbin. I am not at all surprised that it currently fails to build because I do not test on Windows, and the bit-rot will be advancing! I should think that it will require minor changes to get it working again. My reason for not supporting Windows as a first-class citizen was actually because I wasn't aware of a good way to install the packages required to build it - Windows could really do with apt, yum or a similar package manager! However, if vcpkg provides that feature for Windows, I think it should be possible to make a Windows CI action on github and ensure that a basic build on Windows will work. That would be really great. The cmake files should define |
I agree, but the function I had issues with is in a LE: I hope I'm not getting this wrong, but I think it was this function: Line 1743 in 0709cc8
|
Ah - I see. testheaders.cpp or process_colourtables.cpp call that function, so process_colourtables.cpp was probably the issue. That is a program that was used as a one-off to generate some code in ColourMap.cpp. It does not need to build normally. I should add a BUILD_UTILS flag in cmake with default value OFF. |
Regarding the CI pipeline, it should work. LE: This can also be applied for Mac and Linux. |
Made a point release v3.3.1 with these changes in. |
I have started a new pipeline and also included Windows this time. |
I'm back. I had only one issue on Windows:
I fixed this by applying the following patch: diff --git a/CMakeLists.txt b/CMakeLists.txt
index bd349bf..e91afdd 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -134,7 +134,7 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DMORPH_FONTS_DIR=\"\\\"${PROJECT_SOURCE
# Lib finding
find_package(OpenCV QUIET) # No longer required for morph::Visual, used by some tests
-if(APPLE)
+if(APPLE OR WIN32)
find_package(OpenGL REQUIRED)
else()
find_package(OpenGL REQUIRED EGL) # GL ES 3.1 used in one gl compute example I tried to install Other than that, everything went well. You can view the pipeline here. |
I've added |
Thanks ! I think everything is done. |
Hi Seb, Please have a look at this: microsoft/vcpkg#43752 (review) |
|
See how you get on with The Beautifully Packaged Release ;) https://github.com/ABRG-Models/morphologica/releases/tag/v3.4 |
Hi Emilian, I am close to getting a vcpkg based Visual studio program to work, using vcpkg from your fork. I have morphologica installed fine with vcpkg install morphologica, and Visual Studio (or perhaps intellisense) has found the headers, but the project doesn't build because VisualStudio is using its built-in vcpkg, not the one I cloned in my home directory. Do you know how to switch it out, so that VisualStudio uses an alternative vcpkg? |
Hi Seb, |
Great thanks! I'll try that |
I think I got the environment set up right (VCPKG_ROOT and PATH) and I re-initialized the project (to generate vcpkg.json and vcpkg-configuration.json) and the build error changed to this.
The line
looks suspect - I wonder if it should read:
|
Update: There's still an issue with VCPKG_ROOT:
|
Hmm, but anyway - that warning said it was in any case ignoring the wrong vcpkg_root and paying attention to the right one. And when I provide a cmd line switch to set it, I get the same non-existence error
|
If I check support when in the vcpkg directory, I get a success message:
|
I think I just need to find the correct content for vcpkg-configuration.json {
"default-registry": {
"kind": "git",
"baseline": "aeb9a78803c8457984b73e6e5ac3b979725fa0d1", // this was already the correct commit
"repository": "https://github.com/rremilian/vcpkg" // changed
},
"registries": [
{
"kind": "artifact",
"location": "https://github.com/microsoft/vcpkg-ce-catalog/archive/refs/heads/main.zip",
"name": "microsoft"
}
]
} |
I'm giving up for tonight. Sorry for the spam! |
So it seems that after
The first call to Any ideas? |
I created a discussion asking for help on this problem microsoft/vcpkg#43857 |
Thanks for your help on the discussion. I can now compile this program: #include <iostream>
#include <morph/vvec.h>
int main()
{
morph::vvec<int> vv = { 1, 2, 4, 8 };
std::cout << "The morph::vvec variable vector contains: " << vv << "\n";
} in Visual Studio 2022, using vcpkg to install the morphologica dependency. I am now trying to get the basic graphical example graph1.cpp to work. I need to find the right way to include the GL header code in Windows. I can already trigger errors simply by changing the program above to: #define __WIN__ // usually passed as compiler flag, after definition in CMake
// may need: -DNOMINMAX /EHsc // Hint from old Windows compiling efforts. /EHsc was to support exception handling
#define NOMINMAX
#define GL3_PROTOTYPES // usually defined in CMake
#define GL_GLEXT_PROTOTYPE //
//#define USE_GLEW may need this. // Previous Windows builds used GLEW. If required, this would be a dependency of the vcpkg I think
#include <morph/Visual.h> // causes compile errors
#include <iostream>
#include <morph/vvec.h>
int main()
{
morph::vvec<int> vv = { 1, 2, 4, 8 };
std::cout << "The morph::vvec variable vector contains: " << vv << "\n";
} I'll see if I can figure out how to do without GLEW, but this is just a heads-up that it may be desirable to add glew as a dependency for the Windows platform. |
This can be done. Give me a heads up if you want me to add it. |
Hi Emilian, I've made a new release (v3.4.1), with housekeeping intended to minimize errors and warnings in Visual Studio whilst always improving the codebase. It would be great if you could update your fork of vcpkg with this new release. Perhaps it doesn't need to go to a PR in vcpkg, as I hope to be able to make a fully Windows compatible release. This intermediate release will help me to get the GL incantations correct to make that happen. |
Hi Seb, Sure. Should I also add glew as a dependency ? |
No need for glew I'm hoping. |
I updated the fork. |
Hi Seb, The pipeline failed on Windows after I updated the port to v3.4.1. Could you please have a look on the PR ? Thanks ! |
I've added a new release v3.4.2 with some modifications to the way incbin.h is handled. |
Hold on - release v3.4.3 coming with another fix that is required in Visual Studio... |
That's done. I'm hoping that with this one, I'll be able to focus on the GL inclusion stuff on Windows. I already managed to get the program to compile and link, but no graphics yet (it crashed). |
Hi Seb, |
Great. I'll pull down the changes and work on the GL stuff. |
I think as an additional effort, I would like to do the following:
I think this will be the best way to get morphologica compiling on Windows and to be able to define a Windows github CI action. |
Hello.
In this commit -> 4051896 -
number_type.h
was moved totrait_tests.h
, butnumber_type.h
it's still mentioned inmorph/CMakeLists.txt
:morphologica/morph/CMakeLists.txt
Line 7 in c0a2e3a
I came across this while trying to add
morphologica
as a port invcpkg
.The build failed saying that
number_type.h
doesn't exist. You can view the entire discussion here.The text was updated successfully, but these errors were encountered: