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

[inih] Upgrade from r51 to r56 #25879

Merged
merged 9 commits into from
Aug 19, 2022
Merged

[inih] Upgrade from r51 to r56 #25879

merged 9 commits into from
Aug 19, 2022

Conversation

SamuelMarks
Copy link
Contributor

Describe the pull request
https://github.com/benhoyt/inih/releases

56

Two updates:

benhoyt/inih#137
benhoyt/inih#140

55

Added "version" to meson.build config: benhoyt/inih#135 (but bumped up to 55 in a subsequent commit, for this release).

54

Mainly benhoyt/inih#134, adding the visibility symbols to the Meson build config, but also other small tweaks to tests and so on.

53

Updates to Meson config:

benhoyt/inih#124 meson: optionally depend on C++
benhoyt/inih#125 meson: enable distro settings by default
benhoyt/inih#126 meson: add static compile args to inih_dep

52

Add INI_CUSTOM_ALLOCATOR to allow using a custom memory allocator. Per the README:

By default when using the heap, the standard library's malloc, free, and realloc functions are used; to use a custom allocator, specify -DINI_CUSTOM_ALLOCATOR=1 (and -DINI_USE_STACK=0). You must define and link functions named ini_malloc, ini_free, and (if INI_ALLOW_REALLOC is set) ini_realloc, which must have the same signatures as the stdlib.h memory allocation functions.

See tests/unittest_alloc.c for an example.

Fixes benhoyt/inih#118.

  • What does your PR fix?

  • Which triplets are supported/not supported? Have you updated the CI baseline?

Yes

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

github-actions[bot]
github-actions bot previously approved these changes Jul 20, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

  • ports/inih/portfile.cmake

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/inih/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions[bot]
github-actions bot previously approved these changes Jul 20, 2022
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Jul 20, 2022
ports/inih/CMakeLists.txt Outdated Show resolved Hide resolved
github-actions[bot]
github-actions bot previously approved these changes Jul 20, 2022
JonLiu1993
JonLiu1993 previously approved these changes Jul 21, 2022
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jul 21, 2022
@JonLiu1993
Copy link
Member

Tested the usage successfully

@BillyONeal
Copy link
Member

This is becoming a relatively large build system replacement. Do you know why we aren't just using upstream's meson one?

@JonLiu1993 JonLiu1993 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 25, 2022
@SamuelMarks
Copy link
Contributor Author

My main goal was debugging why it was getting my #define'd or target_compile_definitions params so I updated the port you already had in order to fix it. No opinion about build systems (us or theirs).

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 5, 2022
Copy link
Contributor

@Thomas1664 Thomas1664 left a comment

Choose a reason for hiding this comment

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

I would like to see that vcpkg_cmake_config_fixup is used + quotes around paths.

ports/inih/portfile.cmake Outdated Show resolved Hide resolved
ports/inih/portfile.cmake Show resolved Hide resolved
ports/inih/portfile.cmake Outdated Show resolved Hide resolved
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I agree with @Thomas1664 's comments

@BillyONeal
Copy link
Member

(I tried to push a fix for you but:
image
)

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 6, 2022
@SamuelMarks
Copy link
Contributor Author

I'll manually make these changes and/or apply the given patch first chance I get (probably 24 hours from now)

In the interim I've given @BillyONeal write access to my vcpkg fork

github-actions[bot]
github-actions bot previously approved these changes Aug 10, 2022
@BillyONeal BillyONeal dismissed stale reviews from github-actions and themself via 17c7d52 August 10, 2022 23:16
github-actions[bot]
github-actions bot previously approved these changes Aug 10, 2022
@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Aug 11, 2022
github-actions[bot]
github-actions bot previously approved these changes Aug 11, 2022
@BillyONeal
Copy link
Member

@SamuelMarks I had to make significant changes in order to get this to pass in CI, because this stealth-merge-conflicted with #26145 which depends on headers this was no longer installing. Please double check that you are happy with these changes.

BillyONeal
BillyONeal previously approved these changes Aug 11, 2022
JonLiu1993
JonLiu1993 previously approved these changes Aug 11, 2022
@JonLiu1993
Copy link
Member

@SamuelMarks, Please double check that you are happy with these changes.

@SamuelMarks
Copy link
Contributor Author

Yes I'm sure this is fine

@BillyONeal BillyONeal dismissed stale reviews from JonLiu1993, github-actions, and themself via 6298b07 August 19, 2022 00:09
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 571a0551ebab159fd460cb1534f6c820fcd6565a -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index fb7c403..2f88646 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -3021,8 +3021,8 @@
       "port-version": 1
     },
     "inih": {
-      "baseline": "51",
-      "port-version": 1
+      "baseline": "56",
+      "port-version": 0
     },
     "iniparser": {
       "baseline": "2020-04-06",

@BillyONeal
Copy link
Member

Thanks for your contribution and thanks for confirming. I resolved the merge conflict and will merge as soon as PR build is green. Thanks!

@BillyONeal BillyONeal merged commit 727bdba into microsoft:master Aug 19, 2022
@BillyONeal BillyONeal deleted the inih branch August 19, 2022 00:15
@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory allocation
4 participants