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

CMake fix #67

Merged
merged 7 commits into from
Nov 21, 2023
Merged

CMake fix #67

merged 7 commits into from
Nov 21, 2023

Conversation

jatkinson1000
Copy link
Member

I believe this closes #14

Namely the comments below the main one regarding install location.
cmake files are now placed at $CMAKE_INSTALL_PREFIX/lib/cmake/FTorch/ allowing CMake to locate things automatically if passed CMAKE_PREFIX_PATH = $CMAKE_INSTALL_PREFIX.

This means we no longer specify the FTorch_DIR argument, instead using CMAKE_PREFIX_PATH as standard.

Regarding the other items in #14:

  • Documentation has no dependencies as it is deployed by a GitHub action
  • Examples exist as standalone items with their own CMake - I would argue that they should not in any way be combined with the main CMakeLists
  • Currently no testing dependencies (or testing 😬) but we can make this a flag option when we write them.

Tested on apple M1 mac to build library, install, and build/run examples 1 and 2.
Would appreciate someone else replicating on a different system.

@jatkinson1000 jatkinson1000 self-assigned this Nov 19, 2023
@jatkinson1000 jatkinson1000 added bug Something isn't working enhancement New feature or request labels Nov 19, 2023
@jatkinson1000 jatkinson1000 added this to the Initial Release milestone Nov 19, 2023
@ElliottKasoar
Copy link
Contributor

Both examples seem to work for me on WSL (Ubuntu 20.04)

I assume we're not updating the work-in-progress example for now?

Worth noting we'll need to update the benchmarking repo too.

@jatkinson1000 jatkinson1000 changed the title Cmake fix CMake fix Nov 20, 2023
Copy link
Collaborator

@SimonClifford SimonClifford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about glob patterns in LDFLAGS suggestions in README.md

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@TomMelt TomMelt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good but not happy with *

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jatkinson1000
Copy link
Member Author

OK, made everything /lib and added a note about alternatives.
If someone can green-light I'll merge.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
jatkinson1000 and others added 2 commits November 21, 2023 13:49
Co-authored-by: ElliottKasoar <45317199+ElliottKasoar@users.noreply.github.com>
@jatkinson1000 jatkinson1000 merged commit a6c57fa into main Nov 21, 2023
1 check passed
@jatkinson1000 jatkinson1000 deleted the cmake-fix branch November 21, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake Improvements
4 participants