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

remove symlinks #191

Merged
merged 2 commits into from
Feb 10, 2025
Merged

remove symlinks #191

merged 2 commits into from
Feb 10, 2025

Conversation

davidkoski
Copy link
Collaborator

@davidkoski davidkoski commented Feb 10, 2025

Removes symlinks from checkout -- just move the directories where they need to be.

This looks massive but it is mostly renames. The interesting pieces:

  • Package.swift Cmlx excludes adjusted
  • mlx-c now built directly in its directory instead of via symlink in include
  • headers from mlx-c copied in to include to get the correct path (for symbol export)
  • update-mlx script updated per above

@davidkoski davidkoski requested a review from awni February 10, 2025 18:55
@@ -31,8 +31,14 @@ let package = Package(
.target(
name: "Cmlx",
exclude: [
// exclude here -- it is part of the include directory (public api)
"mlx-c",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously this was picked up via the symlink in include, but now we will build it directly.

// example code + mlx-c distributed
"mlx-c/examples",
"mlx-c/mlx/c/distributed.cpp",
"mlx-c/mlx/c/distributed_group.cpp",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't build these

@@ -10,6 +10,10 @@ then
exit 1
fi

# copy mlx-c headers to build area
rm -f Source/Cmlx/include/mlx/c/*
cp Source/Cmlx/mlx-c/mlx/c/*.h Source/Cmlx/include/mlx/c
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have a symlink in include any more -- just copy these headers in so we get the proper directory structure.

Copy link
Member

@awni awni 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, thanks!

@davidkoski davidkoski merged commit b990c58 into ml-explore:main Feb 10, 2025
3 checks passed
@davidkoski davidkoski deleted the no-symlink branch February 10, 2025 20:56
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