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.

Sign up for GitHub

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

Jump to bottom

include: remove scylla-rust-wrapper/extern/cassandra.h #196

Merged
merged 2 commits into from
Oct 21, 2024
Merged

include: remove scylla-rust-wrapper/extern/cassandra.h #196

merged 2 commits into from
Oct 21, 2024

Conversation

muzarski
Copy link
Collaborator

@muzarski muzarski commented Oct 17, 2024 •
edited
Loading

Fix: #116

I updated build.rs so it now uses original include/cassandra.h file to generate bindings.
Thanks to that, we can remove the needless scylla-rust-wrapper/extern/cassandra.h file.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have implemented Rust unit tests for the features/changes introduced.
  • [ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • [ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

@muzarski muzarski self-assigned this Oct 17, 2024
@muzarski muzarski mentioned this pull request Oct 17, 2024
Merged
4 tasks
@Lorak-mmk
Copy link
Collaborator

Could you, as part of this PR, find out why there need to be 2 of those files, and describe it in the comment somewhere?

@muzarski
Copy link
Collaborator Author

Could you, as part of this PR, find out why there need to be 2 of those files, and describe it in the comment somewhere?

Time to dig into the depths of Cmake files. Wish me luck.

@muzarski
Copy link
Collaborator Author

Could you, as part of this PR, find out why there need to be 2 of those files, and describe it in the comment somewhere?

So it just looks like build.rs makes use of scylla-rust-wrapper/extern/cassandra.h to generate bindings. But as I tested locally, removing scylla-rust-wrapper/extern/cassandra.h, and using ../include/cassandra.h in build.rs works as well.

So we can either retain a symlink in extern/cassandra.h or remove it and adjust the paths in build.rs to point to the original file. WDYT?

@Lorak-mmk
Copy link
Collaborator

I think it's fine to modify build.rs

@muzarski
build: use cassandra.h from top-level include directory
0bd5e0d 
Currently, we use two cassandra.h files during build.
- include/cassandra.h -> an original file, copied from cpp-driver
- scylla-rust-wrapper/extern/cassandra.h -> a copy of the above file,
  used to generate bindings in build.rs.

Having both of the files is error-prone, since we need to remember
to update both of the files in case of some changes. We also need to
make sure that their contents are the same.

In this commit, I update `build.rs` to point to original cassandra.h
file (which is out of the scope of Cargo project, but it does not
seem to be a problem).
@muzarski
remove scylla-rust-wrapper/extern/cassandra.h
86faf78 
Now, there is no need to have two cassandra.h files
@muzarski muzarski force-pushed the caassandra-h-symlink branch from 6a963a7 to 86faf78 Compare October 21, 2024 10:18
@muzarski muzarski changed the title include: make cassandra.h in scylla-rust-wrapper/extern a symlink include: remove scylla-rust-wrapper/extern/cassandra.h Oct 21, 2024
@muzarski
Copy link
Collaborator Author

I think it's fine to modify build.rs

Done. Updated the description and title accordingly.

@muzarski muzarski merged commit b23b531 into scylladb:master Oct 21, 2024
11 checks passed
@muzarski muzarski mentioned this pull request Nov 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wprzytula wprzytula wprzytula approved these changes

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

@dkropachev dkropachev Awaiting requested review from dkropachev

Assignees

@muzarski muzarski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Avoid redundant cassandra.h file
3 participants
@muzarski @Lorak-mmk @wprzytula