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

number_type.h it's still mentioned in morph/CMakeLists.txt #336

Open
rremilian opened this issue Feb 12, 2025 · 44 comments
Open

number_type.h it's still mentioned in morph/CMakeLists.txt #336

rremilian opened this issue Feb 12, 2025 · 44 comments
Assignees

Comments

@rremilian
Copy link

Hello.
In this commit -> 4051896 - number_type.h was moved to trait_tests.h, but number_type.h it's still mentioned in morph/CMakeLists.txt:

FILES quaternion.h tools.h BezCoord.h BezCurve.h BezCurvePath.h ReadCurves.h AllocAndRead.h MorphDbg.h mathconst.h MathAlgo.h MathImpl.h geometry.h number_type.h Hex.h HexGrid.h hexyhisto.h CartGrid.h histo.h keys.h Grid.h HdfData.h Process.h RD_Base.h DirichVtx.h DirichDom.h ShapeAnalysis.h NM_Simplex.h Rect.h Anneal.h Config.h vec.h vvec.h mat22.h mat33.h mat44.h colour.h ColourMap.h ColourMap_Lists.h lenthe_colormap.hpp colourmaps_cet.h colourmaps_crameri.h scale.h Random.h rngd.h rng.h rngs.h RecurrentNetworkTools.h RecurrentNetwork.h range.h Winder.h trait_tests.h base64.h unicode.h Mnist.h bootstrap.h rapidxml.hpp rapidxml_iterators.hpp rapidxml_print.hpp rapidxml_utils.hpp flags.h version.h

I came across this while trying to add morphologica as a port in vcpkg.
The build failed saying that number_type.h doesn't exist. You can view the entire discussion here.

@sebjameswml
Copy link
Collaborator

sebjameswml commented Feb 12, 2025

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

@rremilian
Copy link
Author

rremilian commented Feb 12, 2025

Thanks for your prompt answer !

A new release would be ideal, but I can work with a commit too by modifying the portfile to download the patch, instead of having it in vcpkg.
As for the nlohmann tweak, vcpkg tries, when possible, to avoid using bundled dependencies. I saw that you also have healpix bundled, but I left as it is since you modified it.
For nlohmann it's also an issue when trying to build morphologica on Windows, since the nlohmann directory is a symlink and, in my case, it was just a file which contained the location where the symlink pointed to (../include/nlohmann).

I would suggest adding some options in the CMakeLists.txt to let you control whether you want to use the dependencies from the system (when possible), or the bundled ones. Also, for vcpkg it will be helpful to have BUILD_TESTS and BUILD_EXAMPLES options to control whether the tests/examples are built or not.

@rremilian
Copy link
Author

I have one more question: are the fonts required only for the examples or are they needed in general ?

@sebjameswml
Copy link
Collaborator

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

@sebjameswml
Copy link
Collaborator

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.

@rremilian
Copy link
Author

Thanks ! I've modified the PR and I'm waiting for the pipeline to finish.

@sebjameswml
Copy link
Collaborator

That's great. Perhaps your vcpkg will make it possible/easy for me to create a Windows CI Github action?

@rremilian
Copy link
Author

rremilian commented Feb 13, 2025

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 morph/tools.h. I don't remember specifically which one, but it was one included in one of the #ifndef __WIN__ statements.
I wasn't sure if morphologica still works on Windows, so I set the support on the vcpkg port to !windows.
If it will help, I can come back with additional output later.

@sebjameswml
Copy link
Collaborator

sebjameswml commented Feb 13, 2025

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 __WIN__ as a compiler flag if building on Windows; if this didn't happen for you, then that is the likely cause of the error you observed.

@rremilian
Copy link
Author

rremilian commented Feb 13, 2025

The cmake files should define __WIN__ as a compiler flag if building on Windows; if this didn't happen for you, then that is the likely cause of the error you observed.

I agree, but the function I had issues with is in a #ifndef __WIN__ block - so if __WIN__ is not defined, the function will be. For me, __WIN__ was defined, and the function was not.

LE: I hope I'm not getting this wrong, but I think it was this function:

void readDirectoryTree (std::vector<std::string>& vec,

@sebjameswml
Copy link
Collaborator

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.

@rremilian
Copy link
Author

rremilian commented Feb 13, 2025

Regarding the CI pipeline, it should work.
You will have to clone vcpkg, run bootstrap_vcpkg.bat and vcpkg.exe install (you will need to create a manifest file with the required dependencies)
You would also need to set the CMAKE_TOOLCHAIN_FILE.

LE: This can also be applied for Mac and Linux.
LE2: You will need a manifest file.

@sebjameswml
Copy link
Collaborator

Made a point release v3.3.1 with these changes in.

@rremilian
Copy link
Author

I have started a new pipeline and also included Windows this time.

@rremilian
Copy link
Author

rremilian commented Feb 13, 2025

I'm back. I had only one issue on Windows:

CMake Error at D:/downloads/tools/cmake-3.30.1-windows/cmake-3.30.1-windows-i386/share/cmake-3.30/Modules/FindPackageHandleStandardArgs.cmake:233 (message):
  Could NOT find OpenGL (missing: EGL)

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 egl from vcpkg, but the config still didn't find it.
I would appreciate your feedback on this. Also, this might be helpful: StackOverflow

Other than that, everything went well. You can view the pipeline here.

@sebjameswml
Copy link
Collaborator

sebjameswml commented Feb 13, 2025

I've added if(APPLE OR WIN32) like you did so that find_package goes looking for EGL only on Linux. This is now in main. EGL is not important for data visualization, it just allows you to run OpenGL compute tasks without having to have a display window open.
Update: Whoops - no - EGL is about being able to support OpenGL ES, which I have done so that I could run both GL compute programs and also data visualization programs on a Raspberry Pi.

@rremilian
Copy link
Author

Thanks ! I think everything is done.
I requested a review.

@rremilian
Copy link
Author

rremilian commented Feb 14, 2025

Hi Seb,

Please have a look at this: microsoft/vcpkg#43752 (review)
Could you please make the changes in order to use the system libraries by default for rapidxml, egl , gl3 and gl (+ any other bundled libraries - jcvoronoi doesn't have a port in vcpkg, but I think it would be good to include this too, maybe it will be added later).
I think it will work to check first if the libraries are present on the system and, if not, to use the bundled ones.
For Debian the packages should be : librapidxml-dev libegl1-mesa-dev libgles2-mesa-dev libegl-dev

@sebjameswml
Copy link
Collaborator

  1. rapidxml This is the easiest to resolve of that list - as you noticed there are Debian packages. I am not sure that there is a brew package for Mac though. Because I use brew package management in the Mac CI runners on github, I may have to keep a copy of rapidxml as a fall-back.
  2. egl/gl3/gl I accept that my approach of bundling the GL headers is not great; I never really loved it, but it was something that worked early on. I'll look at fixing it. Getting GL headers right cross-platform is quite hard.
  3. jcvoronoi This is another piece of header code that I have significantly modified from the original (I used a morphologica mathematical vector type in the code to generate 2D Voronoi patterns so that I could subsequently extend them into '2.5D surfaces'. So this one, like the HEALpix code, has to stay bundled.

@sebjameswml sebjameswml self-assigned this Feb 14, 2025
@sebjameswml
Copy link
Collaborator

See how you get on with The Beautifully Packaged Release ;) https://github.com/ABRG-Models/morphologica/releases/tag/v3.4

@sebjameswml
Copy link
Collaborator

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?

@rremilian
Copy link
Author

Hi Seb,
I am not sure if it will work, but could you try to run this in your cloned directory -> .\vcpkg.exe integrate install ?
And also, I think you should set the VCPKG_ROOT variable and add it to PATH. Have a look here -> Microsoft Docs

@sebjameswml
Copy link
Collaborator

Great thanks! I'll try that

@sebjameswml
Copy link
Collaborator

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.

Build started at 22:41...
1>------ Build started: Project: Morphytest, Configuration: Debug x64 ------
1>Installing vcpkg dependencies to C:\Users\sebj\source\repos\Morphytest\vcpkg_installed\x64-windows\
1>"C:\Users\sebj\vcpkg\vcpkg.exe" install  --x-wait-for-lock --triplet "x64-windows" --vcpkg-root "C:\Users\sebj\vcpkg\\" "--x-manifest-root=C:\Users\sebj\source\repos\Morphytest\\" "--x-install-root=C:\Users\sebj\source\repos\Morphytest\vcpkg_installed\x64-windows\\"
1>Fetching registry information from https://github.com/microsoft/vcpkg (HEAD)...
1>EXEC : error : morphologica does not exist
1>C:\Users\sebj\vcpkg\scripts\buildsystems\msbuild\vcpkg.targets(183,5): error MSB3073: The command ""C:\Users\sebj\vcpkg\vcpkg.exe" install  --x-wait-for-lock --triplet "x64-windows" --vcpkg-root "C:\Users\sebj\vcpkg\\" "--x-manifest-root=C:\Users\sebj\source\repos\Morphytest\\" "--x-install-root=C:\Users\sebj\source\repos\Morphytest\vcpkg_installed\x64-windows\\" " exited with code 1.
1>Done building project "Morphytest.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 0 up-to-date, 0 skipped ==========
========== Build completed at 22:41 and took 03.126 seconds ==========

The line

1>Fetching registry information from https://github.com/microsoft/vcpkg (HEAD)...

looks suspect - I wonder if it should read:

1>Fetching registry information from https://github.com/rremilian/vcpkg (morphologica-v3.2)...

@sebjameswml
Copy link
Collaborator

Update: There's still an issue with VCPKG_ROOT:

PS C:\Users\sebj\source\repos\Morphytest> vcpkg x-check-support morphologica
warning: The vcpkg C:\Users\sebj\vcpkg\vcpkg.exe is using detected vcpkg root C:\Users\sebj\vcpkg and ignoring mismatched VCPKG_ROOT environment value C:\Program Files\Microsoft Visual Studio\2022\Community\VC\vcpkg. To suppress this message, unset the environment variable or use the --vcpkg-root command line switch.
Fetching registry information from https://github.com/microsoft/vcpkg (HEAD)...
error: while looking for morphologica:x64-windows:
error: morphologica does not exist
PS C:\Users\sebj\source\repos\Morphytest>

@sebjameswml
Copy link
Collaborator

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

C:\Users\sebj\source\repos\Morphytest> vcpkg x-check-support morphologica --vcpkg-root="C:\Users\sebj\vcpkg"
Fetching registry information from https://github.com/microsoft/vcpkg (HEAD)...
error: while looking for morphologica:x64-windows:
error: morphologica does not exist
PS C:\Users\sebj\source\repos\Morphytest>

@sebjameswml
Copy link
Collaborator

If I check support when in the vcpkg directory, I get a success message:

PS C:\Users\sebj\vcpkg> vcpkg x-check-support morphologica --vcpkg-root="C:\Users\sebj\vcpkg"
Port morphologica[core]:x64-windows is supported.

@sebjameswml
Copy link
Collaborator

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"
    }
  ]
}

@sebjameswml
Copy link
Collaborator

I'm giving up for tonight. Sorry for the spam!

@sebjameswml
Copy link
Collaborator

So it seems that after vcpkg new --application, morphologica no longer exists. Starting in a directory Morphytest2 which contains no *.json files:

PS C:\Users\sebj\source\repos\Morphtest2> vcpkg x-check-support morphologica --vcpkg-root="C:\Users\sebj\vcpkg"
Port morphologica[core]:x64-windows is supported.
PS C:\Users\sebj\source\repos\Morphtest2> vcpkg new --application --vcpkg-root="C:\Users\sebj\vcpkg"
PS C:\Users\sebj\source\repos\Morphtest2> vcpkg x-check-support morphologica --vcpkg-root="C:\Users\sebj\vcpkg"
Fetching registry information from https://github.com/microsoft/vcpkg (HEAD)...
error: while looking for morphologica:x64-windows:
error: morphologica does not exist
PS C:\Users\sebj\source\repos\Morphtest2>

The first call to vcpkg x-check-support morphologica succeeds, then the second, after vcpkg new --application fails.

Any ideas?

@sebjameswml
Copy link
Collaborator

sebjameswml commented Feb 15, 2025

I created a discussion asking for help on this problem microsoft/vcpkg#43857

@sebjameswml
Copy link
Collaborator

sebjameswml commented Feb 15, 2025

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.

@rremilian
Copy link
Author

This can be done. Give me a heads up if you want me to add it.

@sebjameswml
Copy link
Collaborator

sebjameswml commented Feb 16, 2025

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.

@rremilian
Copy link
Author

Hi Seb,

Sure. Should I also add glew as a dependency ?

@sebjameswml
Copy link
Collaborator

No need for glew I'm hoping.

@rremilian
Copy link
Author

I updated the fork.

@rremilian
Copy link
Author

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 !

@sebjameswml
Copy link
Collaborator

I've added a new release v3.4.2 with some modifications to the way incbin.h is handled.

@sebjameswml
Copy link
Collaborator

Hold on - release v3.4.3 coming with another fix that is required in Visual Studio...

@sebjameswml
Copy link
Collaborator

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).

@rremilian
Copy link
Author

rremilian commented Feb 16, 2025

Hi Seb,
Thanks ! The pipeline passed with no issues. Now we wait for the review.

@sebjameswml
Copy link
Collaborator

Great. I'll pull down the changes and work on the GL stuff.

@sebjameswml
Copy link
Collaborator

I think as an additional effort, I would like to do the following:

  1. Clone morphologica on Windows
  2. Use vcpkg to bring in the dependencies (nlohmann-json, rapidxml, OpenGL, etc etc)
  3. Execute a cmake-directed build of morphologica, using the Visual Studio compiler

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.

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

2 participants