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

wip: libtypec: init at 0.5.0 (cmake help needed) #270763

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

Conversation

colemickens
Copy link
Member

Description of changes

Hi, I need help with this.

Out standing issues:

  1. Am I doing the right thing to build a cmake subdir that's not included in the parent project?
  2. Any suggestions on what to do about the utils binaries? They link against the local relative path to libtypec, which is incorrect and fails the reference check at the end of the build.

Init libtypec at 0.4.0.

project url: https://github.com/Rajaram-Regupathy/libtypec
description:

“libtypec” is aimed to provide a generic interface abstracting all platform complexity for user space to develop tools for efficient USB-C port management. The library can also enable development of diagnostic and debug tools to debug system issues around USB-C/USB PD topology.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Priorities

Add a 👍 reaction to pull requests you find important.

@NickCao
Copy link
Member

NickCao commented Nov 29, 2023

Am I doing the right thing to build a cmake subdir that's not included in the parent project?

The current best practice seems to be cd into the subdirectory in preConfigure.

Any suggestions on what to do about the utils binaries

There should be some cmake options controlling the rpath handling behavior, if that fails, patchelf is also fine.

@colemickens
Copy link
Member Author

Regarding the cd tip, to clarify, I do want to build the actual lib too. It'd probably be appropriate for this package to have out and bin as outputs.

There should be some cmake options controlling the rpath handling behavior,

🤔 hmm, ok, that gives me something to (rip)grep for anyway. Thanks @NickCao

@NickCao
Copy link
Member

NickCao commented Nov 29, 2023

I do want to build the actual lib too.

Or split it into two derivations?

@colemickens
Copy link
Member Author

It does sound easier, but I feel like the multi-output support was made for something like this. (Happy to be wrong, I just can't of any nixpkgs pkgs that have two derivations that map to the same source (well, for something small like this anyway).

@NickCao
Copy link
Member

NickCao commented Nov 29, 2023

I'm no cmake expert, but this works. (so many cmake crimes going on...)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index b7f617f..a46f096 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -36,3 +36,5 @@ configure_file(

 install(FILES ${CMAKE_BINARY_DIR}/libtypec.pc
     DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/pkgconfig)
+
+add_subdirectory(utils)
diff --git a/utils/CMakeLists.txt b/utils/CMakeLists.txt
index b726a54..87a71e6 100644
--- a/utils/CMakeLists.txt
+++ b/utils/CMakeLists.txt
@@ -4,13 +4,13 @@ project(lstypec VERSION 0.4.0)
 include(CTest)
 enable_testing()

-get_filename_component(libtypec_PROJECT_ROOT "${CMAKE_CURRENT_LIST_DIR}/../.." ABSOLUTE)
 add_executable(lstypec lstypec.c names.c)
 add_executable(typecstatus typecstatus.c names.c)

-target_link_libraries(lstypec "${CMAKE_CURRENT_LIST_DIR}/../bin/libtypec.so" udev)
-target_link_libraries(typecstatus "${CMAKE_CURRENT_LIST_DIR}/../bin/libtypec.so" udev)
+target_link_libraries(lstypec libtypec udev)
+target_link_libraries(typecstatus libtypec udev)

+install(TARGETS lstypec typecstatus)

 set(CMAKE_C_FLAGS "-g -O2 -fstack-protector-strong -Wformat=1 -Werror=format-security -Wdate-time -fasynchronous-unwind-tables -D_FORTIFY_SOURCE=2")
 set(CPACK_PROJECT_NAME ${PROJECT_NAME})

@colemickens
Copy link
Member Author

Thanks @NickCao! That's great.

I do still feel a bit weird given that this strikes me as a lib that the author expects to be used in other things, so I feel like this still needs one more pass to split it into out/bin. (Not asking you to do this, just noting for the PR's sake)

@colemickens
Copy link
Member Author

I took a naive stab at it, but it complains about references in a new commit. I'm also not sure if it should be out/lib, lib/bin, or out/bin.

error: cycle detected in build of '/nix/store/12val53iwgjwbq0gmdd5xn1m75gm4cb2-libtypec-0.4.0.drv' in the references of output 'bin' from output 'out'

@colemickens colemickens changed the title wip: libtypec: init at 0.4.0 (cmake help needed) wip: libtypec: init at 0.5.0 (cmake help needed) Mar 31, 2024
@colemickens
Copy link
Member Author

Updated this to 0.5.0 but it still doesn't naively build with cmake. Going to share this with the upstream author and see if they have ideas.

@NickCao
Copy link
Member

NickCao commented May 31, 2024

0.5.1 is out, does it work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants