[librdata] Add new recipe library #26024

shun2wang · 2024-11-24T09:48:25Z

Summary

Add recipe: librdata

Motivation

Add new recipe of librdata

Details

Read the contributing guidelines
Checked that this PR is not a duplicate: list of PRs by recipe
Tested locally with at least one configuration using a recent version of Conan

valgur · 2024-11-25T12:04:44Z

recipes/librdata/all/conanfile.py

+    name = "librdata"
+    version = "0.1"
+    description = "librdata is a library for read and write R data frames from C"
+    license = "CDDL-1.0", "LGPL-2.1-only"


Not mandatory, but use a single expression with an AND or OR clause for the licenses, if possible, for clarity.

valgur · 2024-11-25T12:06:23Z

recipes/librdata/all/conanfile.py

+            self.requires("zlib/1.2.13")
+            self.requires("xz_utils/5.6.3")


Should use version ranges for these - see the CCI docs for details.

valgur · 2024-11-25T12:10:02Z

recipes/librdata/all/conanfile.py

+        git = Git(self)
+        git.clone(url="https://github.com/WizardMac/librdata.git", target=".")


This is not allowed on CCI - the sources used by the recipe must be immutable. Use a source archive from a tagged release or a source archive from a specific commit instead. I recommend taking a look at the recipe templates under docs/.

This upstream repo is not has any release and tag for version control but can I just use HEAD.zip from repo?

Thanks for your review.

valgur · 2024-11-25T12:10:42Z

recipes/librdata/all/conanfile.py

+        env = VirtualBuildEnv(self)
+        env.generate()


No longer needed with Conan v2.

valgur · 2024-11-25T12:12:24Z

recipes/librdata/all/conanfile.py

+        tc = AutotoolsToolchain(self)
+        tc.generate()
+        tc = AutotoolsDeps(self)
+        if self.options.with_zip:
+            tc.variables["with_zip"] = True
+        tc.generate()


This is broken. You will have to pass the configure flag via AutotoolsToolchain.

valgur · 2024-11-25T12:13:46Z

recipes/librdata/all/conanfile.py

+        fix_apple_shared_install_name(self)
+
+    def package_info(self):
+        self.cpp_info.set_property("pkg_config_name", "readstat")


Is the .pc name of the library really that different from the project/recipe name?

valgur · 2024-11-25T12:14:54Z

recipes/librdata/all/test_package/test_package.c

+    printf("Read table: %s\n", name);
+
+    return 0;
+}


You should probably add a main() function for this to be a proper executable test.

recipes/librdata/all/test_package/conanfile.py

uilianries

@shun2wang Thank you for your PR! I see librdata is packaged by Ubuntu and Debian as well!

In case you PR is still in WIP, please, move it to draft.

Please, take a look in my review!

uilianries · 2024-11-26T09:15:56Z

recipes/librdata/all/conandata.yml

+  "0.1":
+    url: "https://github.com/WizardMac/librdata/archive/refs/heads/master.zip"
+    sha256: "0f3eb4a386b9dbf9bdebba424061f7e34a0e1ec27fa4e631257c3be6ca450c57"


Suggested change

"0.1":

url: "https://github.com/WizardMac/librdata/archive/refs/heads/master.zip"

sha256: "0f3eb4a386b9dbf9bdebba424061f7e34a0e1ec27fa4e631257c3be6ca450c57"

"0.0.0.cci.20231003":

url: "https://github.com/WizardMac/librdata/archive/33bd276ecb0bbcd8997ccc71a544149b3da0d940.tar.gz"

sha256: "fc3e0d655c21d4b5144a0ad73f79a249903b5dde4dc8f7091cd0393d214294a2"

Please, read https://github.com/conan-io/conan-center-index/blob/master/docs/faqs.md#what-version-should-packages-use-for-libraries-without-official-releases for extra info

uilianries · 2024-11-26T09:18:26Z

recipes/librdata/all/conanfile.py

+        # git = Git(self)
+        # git.clone(url="https://github.com/WizardMac/librdata.git", target=".")


Suggested change

# git = Git(self)

# git.clone(url="https://github.com/WizardMac/librdata.git", target=".")

Git feature is not used in CCI. Instead, download specific tar/zip file from the upstream.

uilianries · 2024-11-26T09:21:21Z

recipes/librdata/all/conanfile.py

+        "with_zip": [True, False],
+    }
+    default_options = {
+        "shared": False,
+        "fPIC": True,
+        "with_zip": False,


Suggested change

"with_zip": [True, False],

}

default_options = {

"shared": False,

"fPIC": True,

"with_zip": False,

}

default_options = {

"shared": False,

"fPIC": True,

I would suggest not adding an option for dependencies. The upstream does not expose an option as well when running configure script. I would suggest including all dependencies by default.

uilianries · 2024-11-26T09:23:37Z

recipes/librdata/all/conanfile.py

+        if self.options.with_zip:
+            self.requires("bzip2/1.0.8")
+            self.requires("zlib/1.2.13")
+            self.requires("xz_utils/5.6.3")


Suggested change

if self.options.with_zip:

self.requires("bzip2/1.0.8")

self.requires("zlib/1.2.13")

self.requires("xz_utils/5.6.3")

self.requires("bzip2/1.0.8")

self.requires("zlib/[>=1.2.11 <2]")

self.requires("xz_utils/[>=5.4.5 <6]")

Please, read https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/dependencies.md#version-ranges for further information about allowed range versions in CCI.

Im sure it should only required when with_zip.

uilianries · 2024-11-26T09:25:04Z

recipes/librdata/config.yml

@@ -0,0 +1,3 @@
+versions:
+  "0.1":


Suggested change

"0.1":

"0.0.0.cci.20231003":

uilianries · 2024-11-26T09:28:13Z

recipes/librdata/all/test_package/test_package.c

+int main(int argc, char const *argv[])
+{
+    rdata_parser_t *parser = rdata_parser_init();
+    if (parser == NULL) {
+        fprintf(stderr, "Failed to initialize rdata_parser.\n");
+        return 1;
+    }
+
+    rdata_parser_free(parser);
+    printf("librdata is successfully included, linked, and used.\n");


Suggested change

int main(int argc, char const *argv[])

{

rdata_parser_t *parser = rdata_parser_init();

if (parser == NULL) {

fprintf(stderr, "Failed to initialize rdata_parser.\n");

return 1;

}

rdata_parser_free(parser);

printf("librdata is successfully included, linked, and used.\n");

int main()

{

rdata_parser_t *parser = rdata_parser_init();

rdata_parser_free(parser);

It does not need to check the method. The idea is checking only linkage and headers.

uilianries · 2024-11-26T09:29:44Z

recipes/librdata/all/conanfile.py

+
+    def package_info(self):
+        suffix = "_i" if is_msvc(self) and self.options.shared else ""
+        self.cpp_info.libs = [f"librdata{suffix}"]


Suggested change

self.cpp_info.libs = [f"librdata{suffix}"]

self.cpp_info.libs = [f"rdata{suffix}"]

The prefix lib should not part of the library name, as it's a standard based on the platform, not the library names itself.

But the library does named as librdata...

uilianries · 2024-11-26T09:32:14Z

recipes/librdata/all/conanfile.py

+
+    def package(self):
+        copy(self, pattern="LICENSE*", dst=os.path.join(self.package_folder, "licenses"), src=self.source_folder)
+        if is_msvc(self):


Have you really tested on Windows? Usually, it requires more when talking about autotools. In case you did not and has no intention, the recipe still can be accepted in CCI, just need more information.

Thanks for your comment!

I really just want to build it for MacOS and Windows instead of Linux, but even so far I haven't found a suitable way to successfully build on Windows. Always fails at the config stage.

I could take a look during the week for windows support. Right now, I don't have access to windows machine, but I'll have by tomorrow. You can take a look in gettext recipe as example: https://github.com/conan-io/conan-center-index/blob/master/recipes/libgettext/all/conanfile.py

uilianries · 2024-11-27T08:28:09Z

recipes/librdata/all/conanfile.py

+
+class Libreadstat(ConanFile):
+    name = "librdata"
+    version = "0.1"


Suggested change

version = "0.1"

Version should not be listed in the conan recipe

uilianries · 2024-11-27T08:56:55Z

@shun2wang I was able to built librdata using MSYS2 on Windows: librdata-0.0.0-windows-msys.log

The upstream uses MinGW in their CI: https://github.com/WizardMac/librdata/blob/master/.github/workflows/build.yml#L45

There is no mention about MSVC support, even in the issues.

I would suggest not supporting MSVC for now, as it's not working and the there is no official support about.

The msys can be supported, you just need to follow the Autotools template or gettext recipe as example.

The ConanCenterIndex CI does not run MSYS, but you still can post a full build log in the comments like I did, the prove that's working.

shun2wang force-pushed the addLibrdata branch 5 times, most recently from 29ad044 to bd661b3 Compare November 25, 2024 03:58

shun2wang changed the title ~~init add librdata library~~ [WIP] init add librdata library Nov 25, 2024

init add librdata library

cd5a1c4

shun2wang force-pushed the addLibrdata branch from bd661b3 to cd5a1c4 Compare November 25, 2024 06:41

valgur suggested changes Nov 25, 2024

View reviewed changes

shun2wang force-pushed the addLibrdata branch from 91998e2 to f37758c Compare November 26, 2024 07:29

clean build

0774bd0

shun2wang force-pushed the addLibrdata branch from f37758c to 0774bd0 Compare November 26, 2024 07:32

autoreconf for build

38227a5

uilianries self-assigned this Nov 26, 2024

uilianries reviewed Nov 26, 2024

View reviewed changes

uilianries added the waiting on contributor label Nov 26, 2024

shun2wang marked this pull request as draft November 26, 2024 09:49

uilianries removed the waiting on contributor label Nov 26, 2024

uilianries changed the title ~~[WIP] init add librdata library~~ [librdata] Add new recipe library Nov 26, 2024

uilianries reviewed Nov 27, 2024

View reviewed changes

uilianries mentioned this pull request Nov 27, 2024

readstat: add new recipe of readstat/1.1.9 #25181

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[librdata] Add new recipe library #26024

[librdata] Add new recipe library #26024

shun2wang commented Nov 24, 2024 •

edited

Loading

valgur Nov 25, 2024

valgur Nov 25, 2024

valgur Nov 25, 2024

shun2wang Nov 25, 2024 •

edited

Loading

valgur Nov 25, 2024

valgur Nov 25, 2024

valgur Nov 25, 2024

valgur Nov 25, 2024

shun2wang Nov 26, 2024

uilianries left a comment

uilianries Nov 26, 2024

uilianries Nov 26, 2024

uilianries Nov 26, 2024

uilianries Nov 26, 2024

shun2wang Nov 26, 2024

uilianries Nov 26, 2024

uilianries Nov 26, 2024

uilianries Nov 26, 2024

shun2wang Nov 26, 2024

uilianries Nov 26, 2024

shun2wang Nov 26, 2024

uilianries Nov 26, 2024

uilianries Nov 27, 2024

uilianries commented Nov 27, 2024

		git = Git(self)
		git.clone(url="https://github.com/WizardMac/librdata.git", target=".")

-  "0.1":
-    url: "https://github.com/WizardMac/librdata/archive/refs/heads/master.zip"
-    sha256: "0f3eb4a386b9dbf9bdebba424061f7e34a0e1ec27fa4e631257c3be6ca450c57"
+  "0.0.0.cci.20231003":
+    url: "https://github.com/WizardMac/librdata/archive/33bd276ecb0bbcd8997ccc71a544149b3da0d940.tar.gz"
+    sha256: "fc3e0d655c21d4b5144a0ad73f79a249903b5dde4dc8f7091cd0393d214294a2"

		# git = Git(self)
		# git.clone(url="https://github.com/WizardMac/librdata.git", target=".")

	self.cpp_info.libs = [f"librdata{suffix}"]
	self.cpp_info.libs = [f"rdata{suffix}"]

[librdata] Add new recipe library #26024

Are you sure you want to change the base?

[librdata] Add new recipe library #26024

Conversation

shun2wang commented Nov 24, 2024 • edited Loading

Summary

Motivation

Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shun2wang Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uilianries left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uilianries commented Nov 27, 2024

shun2wang commented Nov 24, 2024 •

edited

Loading

shun2wang Nov 25, 2024 •

edited

Loading