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

[ports/ed25519/CMakeLists.txt] Install all headers #35143

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SamuelMarks
Copy link
Contributor

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Not sure if I'm meant to manually change "port-version": 0?

Anyway these headers all need to be installed as some packages, for example, depend on ge.h so that needs to be installed.

@JonLiu1993
Copy link
Member

@SamuelMarks, thanks for your PR, plesase also add "port-version: 1" to vcpkg.json file.

@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Nov 16, 2023
@JonLiu1993 JonLiu1993 marked this pull request as draft November 16, 2023 06:19
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

ed25519:x64-linux:/include/ed25519.h
ed25519:x64-linux:/include/fe.h
ed25519:x64-linux:/include/fixedint.h
ed25519:x64-linux:/include/ge.h
ed25519:x64-linux:/include/precomp_data.h
ed25519:x64-linux:/include/sc.h
ed25519:x64-linux:/include/sha512.h

Not acceptable. Some filepaths (e.g. include/sha512.h) shouldn't be owned by this port.
It might be possible to move the private headers to a s subdir, but does it help those consumers?

@JonLiu1993
Copy link
Member

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@SamuelMarks
Copy link
Contributor Author

@dg0yt @JonLiu1993 How about just these four headers?

Otherwise will have to figure out a pretty big patch file to namespace everything here; and another patch to namespace everything in the callee library (that I don't maintain).

@SamuelMarks SamuelMarks marked this pull request as ready for review November 16, 2023 22:40
@dg0yt
Copy link
Contributor

dg0yt commented Nov 17, 2023

Hm, I'm still mildly opposed to putting private headers into the main include dir, for one unknown use case.
But someone else must decide.

@JonLiu1993
Copy link
Member

Hm, I'm still mildly opposed to putting private headers into the main include dir, for one unknown use case. But someone else must decide.

@SamuelMarks, One thing I'm sure of is that if you want to add a private header file in the main include dir, you need to get the author's permission from upstream.

@dg0yt
Copy link
Contributor

dg0yt commented Nov 17, 2023

There are two separate questions:

  • whether to install private headers: upstream.
  • whether to install particular files directly to include: integrator (vcpkg).

@JonLiu1993
Copy link
Member

Ping @SamuelMarks for response.

@SamuelMarks
Copy link
Contributor Author

I've limited the included headers to 4. If you want it to be namespaced ask?

@JonLiu1993
Copy link
Member

I've limited the included headers to 4. If you want it to be namespaced ask?

@SamuelMarks, You can ask the upstream if it agrees to install the private header file?

@JonLiu1993 JonLiu1993 added the depends:upstream-changes Waiting on a change to the upstream project label Dec 13, 2023
@JonLiu1993 JonLiu1993 removed the depends:upstream-changes Waiting on a change to the upstream project label Feb 27, 2024
@JonLiu1993
Copy link
Member

This upstream repository is unmaintained, and issues submitted upstream have not been responded to for a long time, so I think we can merge this PR.

@JonLiu1993
Copy link
Member

@SamuelMarks, please merge this PR to master, thanks.

@SamuelMarks
Copy link
Contributor Author

@JonLiu1993 Did that yesterday

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Feb 29, 2024
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 5, 2024
@vicroms
Copy link
Member

vicroms commented Mar 7, 2024

@SamuelMarks

We took a look at the headers and noticed that the public API is contained to just ed25519.h. All other headers contain only private implementation details and should not be installed.

You mentioned that some packages require ge.h, what packages do you mean exactly? and why do they need access to the private implementation details?

@vicroms vicroms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 8, 2024
@@ -53,7 +53,7 @@ write_basic_package_version_file(
COMPATIBILITY SameMajorVersion
)
install(FILES "${VERSION_FILE_PATH}" DESTINATION "share/unofficial-${PROJECT_NAME}")
install(FILES "src/ed25519.h" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
install(FILES "src/ed25519.h" "src/fe.h" "src/fixedint.h" "src/ge.h" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
install(FILES "src/ed25519.h" "src/fe.h" "src/fixedint.h" "src/ge.h" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
install(FILES "src/ed25519.h" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vicroms That's what I had before; but the library I am integrated vcpkg into uses these other headers directly and I don't want to edit their codebase [source or header files] in order to support the platforms vcpkg supports.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is still the suggestion to put the private headers into a unique subdir. This is one change for that project (to use an additional include dir), instead of having the private headers in everybody's search path.
(And for specific single-user requirements, their are overlay ports and other customization points.)

Copy link
Member

Choose a reason for hiding this comment

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

The vcpkg team agrees with @dg0yt's points:

  • Moving the private headers into a subdirectory (e.g.: orlp-ed25519/*.h) would be acceptable.
  • Forcing other users to deal with the consequences of a project deciding to take a dependency on private headers not meant for consumption is not acceptable.
  • Users can override the behavior by using overlay ports with the changes you have made here.

Please let me know if this resolution is acceptable to you.

@vicroms vicroms marked this pull request as draft March 8, 2024 07:38
@JonLiu1993 JonLiu1993 removed the info:reviewed Pull Request changes follow basic guidelines label Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants