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

[nanojsonc] Add new port #34909

Merged
merged 44 commits into from
Jan 17, 2024
Merged

[nanojsonc] Add new port #34909

merged 44 commits into from
Jan 17, 2024

Conversation

saadshams
Copy link
Contributor

No description provided.

ports/nanojson/vcpkg.json Outdated Show resolved Hide resolved
@JonLiu1993 JonLiu1993 changed the title Nanojson update [nanojson] Add new port Nov 6, 2023
@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 6, 2023
ports/nanojson/COPYING Outdated Show resolved Hide resolved
@JonLiu1993 JonLiu1993 marked this pull request as draft November 6, 2023 06:39
@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.

saadshams and others added 3 commits November 6, 2023 20:59
Co-authored-by: Alexander Neumann <[email protected]>
Co-authored-by: JonLiu1993 <[email protected]>
Co-authored-by: JonLiu1993 <[email protected]>
@saadshams saadshams marked this pull request as ready for review November 7, 2023 02:05
@dg0yt
Copy link
Contributor

dg0yt commented Nov 7, 2023

IMO the port should be named saadshams-nanojson. The name is not unique, and this upstream isn't dominant.
https://github.com/search?q=nanojson&type=repositories

The COPYING file doesn't belong into the vcpkg repo.

@saadshams
Copy link
Contributor Author

IMO the port should be named saadshams-nanojson. The name is not unique, and this upstream isn't dominant. https://github.com/search?q=nanojson&type=repositories

The COPYING file doesn't belong into the vcpkg repo.

You mean should I delete the COPYING file or LICENSE file?

@dg0yt
Copy link
Contributor

dg0yt commented Nov 7, 2023

You mean should I delete the COPYING file or LICENSE file?

The file LICENSE should be removed from this PR to vcpkg.

@saadshams
Copy link
Contributor Author

saadshams commented Nov 8, 2023

Another sign of quality is the badge for the CI Pipeline, passing with flying colors, notably Windows.

Platforms include:

  1. Windows
  2. Ubuntu (gcc)
  3. Ubuntu (clang)

See the badge. "CMake on multiple platforms passing" on the readme.

Please review.

@JonLiu1993
Copy link
Member

None of the ci tests have been successful so far.

CMake Error at /mnt/vcpkg-ci/installed/x64-linux/share/vcpkg-cmake/vcpkg_cmake_configure.cmake:30 (message):
  SOURCE_PATH must be set
Call Stack (most recent call first):
  ports/nanojson/portfile.cmake:9 (vcpkg_cmake_configure)
  scripts/ports.cmake:168 (include)

@dg0yt
Copy link
Contributor

dg0yt commented Nov 30, 2023

The upstream cmake config installation is broken. Documentation or a good example would help.
https://github.com/saadshams/nanojsonc/blob/97ae7012e88da1368534f7b6ac162c363000ec6f/CMakeLists.txt#L23-L27

If there is nothing else left for the config to do (components, versions, dependencies), it might be as simple as

 install(EXPORT nanojsoncTargets
-        FILE nanojsoncTargets.cmake
-        NAMESPACE nanojsoncTargets::
-        DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/nanojsoncTargets
+        FILE nanojsoncConfig.cmake
+        NAMESPACE nanojsonc::
+        DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/nanojsonc
 )

Assuming the CMake package is nanojsonc:

  • nanojsoncTargets is the "export set" in CMake, an identifier for data handed over between cmake commands.
  • There are various options for the config destination, but Targets isn't expected to be part of it.
  • Targets isn't expected to be part of the namespace.
  • There must be a <Pkg>Config.cmake or <pkg>-config.cmake installed to the desired destination.

@saadshams
Copy link
Contributor Author

saadshams commented Dec 28, 2023

@dg0yt
I have updated the upstream cmake config as per the suggestion. Please check again.

Yes, there's not much this C library is supposed to do; it's a static library with minimalistic configuration, just link and compile for the consumers.

There must be a Config.cmake or -config.cmake installed to the desired destination.

Where do I place this nanojsonc-config.cmake and what will be the contents of it?
I looked at the other projects and created build/config.cmake.in. Link

  1. I'm not familiar with the usage, so I'll need help with the config.cmake.in (I copied from other port).
  2. please check install in my upstream CMakeLists.txt.

Progress

  1. vckpkg install is successful, and I see compiled binaries for nanojsonc under both debug and lib folder.
  2. Headers are getting copied. I see include/nanojsonc/parser.h
  3. License and usage are copied to the share/nanojsonc folder

Issues: For the user of the library.
CMake Error at CMakeLists.txt:8 (find_package): find_package(nanojsonc CONFIG REQUIRED)

Please advise for any changes you see fit. Happy holidays.

P.S. Please note that it's a C-only library (not c++)

@saadshams
Copy link
Contributor Author

saadshams commented Dec 30, 2023

@dg0yt @JonLiu1993 @Neumann-A

Gentlemen, Can you please check what's broken? One more push, please; we are almost there.

library -> CMakeLists

cmake_minimum_required(VERSION 3.26)
project(nanojsonc C)

set(CMAKE_C_STANDARD 11)

add_library(nanojsonc src/object.c src/array.c)
target_compile_features(nanojsonc PRIVATE c_std_11)

include(GNUInstallDirs)
include(CMakePackageConfigHelpers)

target_include_directories(nanojsonc PUBLIC
        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
        $<INSTALL_INTERFACE:include>)

# VCPKG
install(TARGETS nanojsonc EXPORT nanojsonc
        ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
        INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
)

configure_package_config_file(build/config.cmake.in 
        ${CMAKE_CURRENT_BINARY_DIR}/nanojsonc-config.cmake
        INSTALL_DESTINATION ${CMAKE_INSTALL_DATADIR}/nanojsonc NO_SET_AND_CHECK_MACRO)

install(FILES
            ${CMAKE_CURRENT_BINARY_DIR}/nanojsonc-config.cmake
            ${CMAKE_CURRENT_BINARY_DIR}/nanojsonc-config-version.cmake
        DESTINATION
            ${CMAKE_INSTALL_DATADIR}/nanojsonc)

install(EXPORT nanojsonc
        FILE nanojsoncConfig.cmake
        NAMESPACE nanojsonc::
        DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/nanojsonc
)

build/config.make.in

@PACKAGE_INIT@

include(${CMAKE_CURRENT_LIST_DIR}/nanojsonc-targets.cmake)

check_required_components(nanojsonc)

Consumer has a problem. Besides, I don't see these generated config files in the share folder etc.

 find_package(nanojsonc CONFIG REQUIRED)
CMake Error at CMakeLists.txt:15 (find_package):
  Could not find a package configuration file provided by "nanojsonc" with
  any of the following names:

    nanojsoncConfig.cmake
    nanojsonc-config.cmake

@saadshams
Copy link
Contributor Author

@dg0yt @JonLiu1993 @Neumann-A

The library is good to go; please publish it.

Follow-up question: What is "git-tree": "3aabebe9cfde27e640f6573331ce668b843b0381", what is the use and where does the value come from? Source: versions/n-nanjosnc.json

@saadshams
Copy link
Contributor Author

@dg0yt @JonLiu1993 @Neumann-A

Hello guys, can you please publish the library. Waiting...

@Neumann-A
Copy link
Contributor

You dont need to ping me. I have zero decision power here in regards to stuff being merged.

@saadshams
Copy link
Contributor Author

You dont need to ping me. I have zero decision-making power here in regards to stuff being merged.

Thanks for the response. I'm not familiar with the structure and decision process; who do I ping?

@JonLiu1993
Copy link
Member

JonLiu1993 commented Jan 17, 2024

@saadshams, sorry for replying to you so late, I will review this PR carefully today.

@JonLiu1993
Copy link
Member

Tested usage successfully by nanojsonc:x64-windows:

Total install time: 3.5 s
The package nanojsonc provides CMake targets:

    find_package(nanojsonc CONFIG REQUIRED)
    target_link_libraries(main PRIVATE nanojsonc::nanojsonc)

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Jan 17, 2024
@saadshams
Copy link
Contributor Author

@JonLiu1993 Thanks, no problem.

For information, I wanted to know where the "git-tree": "xxx" is coming from in the versions file?

@JonLiu1993
Copy link
Member

@JonLiu1993 Thanks, no problem.

For information, I wanted to know where the "git-tree": "xxx" is coming from in the versions file?

You can refer to this document and Ping me if you have any questions.

@JavierMatosD JavierMatosD merged commit 234d20e into microsoft:master Jan 17, 2024
15 checks passed
@saadshams
Copy link
Contributor Author

Thanks. In case of version upgrades, I have the following steps outlined; please let me know if I'm missing a step.

  1. Update the code on the git and create a new tag on the upstream.
  2. Update the ports/nanojsonc/portfile.cmake : REF and SHA512 values
  3. Update versions/n-nanjosnc.json: git-tree value

@JonLiu1993
Copy link
Member

About your second step,these are some steps I summarized as a maintainer. Of course, you as a user don’t need to be so complicated, but they can give you some reference:

  1. First, check the points that need to be modified in the vcpkg.json file:

              1. First check whether the most basic fields in the vcpkg.json file are present, including Source, Version, and Description. In addition, if Homepage is missing, the user needs to be reminded to add it.
    
              2. Port update requires setting Port-Version to 0 or deleting it.
    
              3. If a port Upstream and we do not support the officially supported triplet, then you need to delete the corresponding triplet in CI.baseline and add it in the CONTROL file
    
                Or add supports to the vcpkg.json file, and add an unsupported message to portfile.cmake.
    
  2. Then you need to check the points that need to be modified in the portfile.cmake file:

              1. Update first needs to pay attention to some deprecated functions (include(vcpkg_common_functions/vcpkg_configure_cmake/vcpkg_install_cmake/...), as well as some requirements
    
                New functions used (vcpkg_replace_string/...), or some special functions vcpkg_check_features that need attention (add FEATURES field)
    
              2. You also need to pay attention to some common functions vcpkg_fixup_pkgconfig() that may be missing (you need to add this function if the .PC file is generated but the path is wrong)
    
              3. There are also some formatting points that need to be paid attention to. For example, the directory behind the port needs to be written as port/${PORT}, and some modification methods that are obviously incorrect in the format need to be pointed out for the user to modify.
    
              4. The use of version, version-server, version-date and version-string fields is easy to confuse.
    
              5. Generally, we only add a "version" field every time we update the port, so we need to pay attention to whether the "version" field in versions/x-/xxx.json is added correctly.
    
  3. The last thing you need to pay attention to is the update where the patch has changed.

              1. If the user's pr update patch is deleted, you need to go to the source to see if the patch has been applied to the sourcr code.
    
              2. If the user adds a new patch in this update and the modified source content is large, the user needs to submit an issue to Upstream to explain the situation.
    
              3. If the user has modified some content in the patch without comments, you need to ask the user.
    
  4. In addition to the content, you also need to check whether the PR title is written in compliance with the specifications.

@saadshams
Copy link
Contributor Author

@JonLiu1993 Thanks for the notes, much appreciated.

Osyotr pushed a commit to Osyotr/vcpkg that referenced this pull request Jan 23, 2024
* Create nanojson (JSON Parser for C/C++)

Event-Driven JSON Parser for C/C++

* Creating nanojson port

Event-Driven JSON Parser for C/C++

* init

* update nanojson

* update sha512

Co-authored-by: Alexander Neumann <[email protected]>

* update version

Co-authored-by: JonLiu1993 <[email protected]>

* update copy license

Co-authored-by: JonLiu1993 <[email protected]>

* update dependencies

Co-authored-by: JonLiu1993 <[email protected]>

* update vcpkg

* update license filename

* deleted license

* update

* update hash

* update

* update

* update

* [saadshams-nanojson] Add new port

* update version

* never agreed to the name, it was published without consent

* rebranded as nanojsonc

* update hash

* update

* update

* update

* format vcpkg.sjon

* update version

* update version

* update

* update

* update

* update

* fix error

* update version

* update

* update

* update

fixed share

* update

* update

* formating vcpkg.json

* update version

* additional empty line

* update version

---------

Co-authored-by: Alexander Neumann <[email protected]>
Co-authored-by: JonLiu1993 <[email protected]>
Co-authored-by: vzhli17 <[email protected]>
TomKatom pushed a commit to TomKatom/vcpkg that referenced this pull request Feb 23, 2024
* Create nanojson (JSON Parser for C/C++)

Event-Driven JSON Parser for C/C++

* Creating nanojson port

Event-Driven JSON Parser for C/C++

* init

* update nanojson

* update sha512

Co-authored-by: Alexander Neumann <[email protected]>

* update version

Co-authored-by: JonLiu1993 <[email protected]>

* update copy license

Co-authored-by: JonLiu1993 <[email protected]>

* update dependencies

Co-authored-by: JonLiu1993 <[email protected]>

* update vcpkg

* update license filename

* deleted license

* update

* update hash

* update

* update

* update

* [saadshams-nanojson] Add new port

* update version

* never agreed to the name, it was published without consent

* rebranded as nanojsonc

* update hash

* update

* update

* update

* format vcpkg.sjon

* update version

* update version

* update

* update

* update

* update

* fix error

* update version

* update

* update

* update

fixed share

* update

* update

* formating vcpkg.json

* update version

* additional empty line

* update version

---------

Co-authored-by: Alexander Neumann <[email protected]>
Co-authored-by: JonLiu1993 <[email protected]>
Co-authored-by: vzhli17 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] nanojson
6 participants