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

[cryptopp] Update port to support new pem-pack option #37360

Closed
wants to merge 2 commits into from
Closed

[cryptopp] Update port to support new pem-pack option #37360

wants to merge 2 commits into from

Conversation

fwosar
Copy link
Contributor

@fwosar fwosar commented Mar 10, 2024

Fixes #31849.

  • 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.

@fwosar
Copy link
Contributor Author

fwosar commented Mar 10, 2024

@microsoft-github-policy-service agree

@fwosar
Copy link
Contributor Author

fwosar commented Mar 10, 2024

Not entirely sure why licensepp fails to build, but it fails with the current version of the port as well. Judging by the project, it appears to require an ancient Crypto++ version (5.6.5) and therefore likely breaks with version 8.9.0 of the library.

@Cheney-W Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Mar 11, 2024
ports/cryptopp/portfile.cmake Outdated Show resolved Hide resolved
ports/cryptopp/portfile.cmake Show resolved Hide resolved
ports/cryptopp/portfile.cmake Outdated Show resolved Hide resolved
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
@Cheney-W Cheney-W marked this pull request as draft March 11, 2024 10:23
@fwosar fwosar marked this pull request as ready for review March 11, 2024 13:27
@fwosar fwosar requested a review from Cheney-W March 11, 2024 13:38
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Mar 13, 2024
Comment on lines +25 to +29
+ fetchcontent_declare(
+ cryptopp-pem
+ GIT_REPOSITORY "https://github.com/noloader/cryptopp-pem"
+ QUIET
+ )
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be fetched in the port rather than being grabbed with FetchContent as the SHA needs to be locked, etc.

)
+ fetchcontent_declare(
+ cryptopp-pem
+ URL "https://github.com/noloader/cryptopp-pem/archive/refs/heads/master.zip"
Copy link
Member

Choose a reason for hiding this comment

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

This fails 'no changes means no changes'. The same port needs to install the same bits if you build it again the next day; building whatever's in master is not OK.

-DUSE_INTERMEDIATE_OBJECTS_TARGET=OFF # Not required when we build static only
-DCMAKE_POLICY_DEFAULT_CMP0063=NEW # Honor "<LANG>_VISIBILITY_PRESET" properties
-DCRYPTOPP_USE_PEM_PACK=${CRYPTOPP_USE_PEM_PACK}
-Dcryptopp-pem_SOURCE_DIR=${PEM_PACK_SOURCE_PATH} # Usually this would be set when cryptopp-cmake downloads the source, but since we don't, we need to provide it instead
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this... you're adding a patch that tries to download from master but this says it doesn't try to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The patch is a diff created from the official cryptopp-cmake project that adds pem-pack support as was requested earlier instead of using their master branch. By default, cryptopp-cmake will download the source for both the pem-pack and Crypto++ themselves, but this behaviour is not used by vcpkg and disabled as those fetch_content directives will only execute when CRYPTOPP_SOURCES is not defined.

I can remove those parts from the patch, but it would mean that the resulting project is no longer a proper replica of the cryptopp-cmake with pem-pack changes cherrypicked.

In that case, all the pem-pack feature in vcpkg would do would be manipulating the source list after copying the required files to the right locations, essentially making the pem-pack support and its related options within cryptopp-cmake redundant.

@BillyONeal BillyONeal marked this pull request as draft March 13, 2024 23:03
@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Mar 13, 2024
@fwosar fwosar closed this Mar 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cryptopp] Pem-pack not being compiled in when selected as an option
3 participants