Skip to content

Conversation

@marfvr
Copy link

@marfvr marfvr commented Nov 13, 2025

This PR replaces occurrences of CMAKE_SOURCE_DIR in the CMake configuration file with CMAKE_CURRENT_SOURCE_DIR. This change would allow the project to be usable as a CMake dependency for other CMake projects.

@github-actions
Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

@nikola-matic
Copy link
Collaborator

Hi @marfvr, OSX and Arch builds are failing. Are you planning on continuing work on this?

@marfvr
Copy link
Author

marfvr commented Dec 8, 2025

Hi @nikola-matic, thank you for your reply.

Yes, I am willing to work on this PR (not immediately, though), as long as it is considered a good change proposal for the project.
Anyway, any of the maintainers/contributors is free to continue from where I left off.

@cameel
Copy link
Collaborator

cameel commented Dec 8, 2025

We already have an old pending PR for this: #15539. We should close one of them.

@cameel
Copy link
Collaborator

cameel commented Dec 8, 2025

Here's the comment I made the last time @matheusaaguiar asked me about that other PR. I guess I should have just posted it there. It's also still relevant here:

Is this change even correct? We use that value e.g. to find the deps/ directory and that's at the top-level. If I understand correctly, CMAKE_CURRENT_SOURCE_DIR is the dir containing the current CMakeFiles.txt so it will be the wrong one unless we're at the top level. I think it passes CI only because we currently don't invoke any of these changed .cmake files from other CMakeFiles.txt. If we did, this change would probably break things. Though I'd have to test this locally to make sure. I'm basing this only on a quick search.

@clonker
Copy link
Member

clonker commented Dec 9, 2025

@cameel you are right with your comment, another one is PROJECT_SOURCE_DIR. However, in this case, the changes should be correct as everything is include(...)-ed. Which doesn't change the CMake directory. It's different from add_subdirectory in that regard. The test failures look unrelated at first glance. Still I am unsure whether we actually want this change, we might rather want to use PROJECT_SOURCE_DIR, which also works if the project is included in other CMake projects as dependency and is more idiomatic in what it refers to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants