diff mbox series

[libubootenv,/,RFC] Replace fw_setenv with symlink to fw_printenv

Message ID 20230301210712.323870-1-mhei@heimpold.de
State Accepted
Headers show
Series [libubootenv,/,RFC] Replace fw_setenv with symlink to fw_printenv | expand

Commit Message

Michael Heimpold March 1, 2023, 9:07 p.m. UTC 
Instead of compiling the same source file twice, just
add a symlink at install time. This is how the "original"
tools also worked.
It also reduces the storage footprint.

Signed-off-by: Michael Heimpold <mhei@heimpold.de>
---

Please take this as RFC: the original tool installed a symlink as fw_setenv and
this tool seems to want to mimic the behavior - or is at least prepared for.

However, it just does not do it - and I don't understand why.
Maybe I missed something - or there just wasn't yet time to implement it that
way?

I don't know - so this is my proposal to change it :-)

 src/CMakeLists.txt | 5 ++---
 src/fw_setenv.c    | 8 --------
 2 files changed, 2 insertions(+), 11 deletions(-)
 delete mode 100644 src/fw_setenv.c

Comments

Stefano Babic March 1, 2023, 10:09 p.m. UTC | #1 
Hi Michael,

On 01.03.23 22:07, Michael Heimpold wrote:
> Instead of compiling the same source file twice, just
> add a symlink at install time. This is how the "original"
> tools also worked.
> It also reduces the storage footprint.
> 
> Signed-off-by: Michael Heimpold <mhei@heimpold.de>
> ---
> 
> Please take this as RFC: the original tool installed a symlink as fw_setenv and
> this tool seems to want to mimic the behavior - or is at least prepared for.
> 
> However, it just does not do it - and I don't understand why.
> Maybe I missed something - or there just wasn't yet time to implement it that
> way?

I let first to have two different tools in case new features are asked, 
and while fw_printenv() will remain simple, fw_setenv could be complex, 
like signing the environment, and so on. That means, I decided first for 
two different tools, and then this was forgotten.

The tools are quite small, the reduced footprint is today negligible.

> 
> I don't know - so this is my proposal to change it :-)

Have you already tested with OE that it is installed as expected ?
> 
>   src/CMakeLists.txt | 5 ++---
>   src/fw_setenv.c    | 8 --------
>   2 files changed, 2 insertions(+), 11 deletions(-)
>   delete mode 100644 src/fw_setenv.c
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 6abdeb4..de4162b 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -21,15 +21,14 @@ SET_TARGET_PROPERTIES(ubootenv PROPERTIES VERSION ${VERSION} SOVERSION ${SOVERSI
>   ADD_LIBRARY(ubootenv_static STATIC ${libubootenv_SOURCES} ${include_HEADERS})
>   SET_TARGET_PROPERTIES(ubootenv_static PROPERTIES OUTPUT_NAME ubootenv)
>   add_executable(fw_printenv fw_printenv.c)
> -add_executable(fw_setenv fw_setenv.c)
>   target_link_libraries(ubootenv z)
>   target_link_libraries(fw_printenv ubootenv)
> -target_link_libraries(fw_setenv ubootenv)
> +add_custom_target(fw_setenv ALL ${CMAKE_COMMAND} -E create_symlink fw_printenv fw_setenv)
>   
>   install (TARGETS ubootenv ubootenv_static DESTINATION ${CMAKE_INSTALL_LIBDIR})
>   install (FILES libuboot.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
>   install (TARGETS fw_printenv DESTINATION ${CMAKE_INSTALL_BINDIR})
> -install (TARGETS fw_setenv DESTINATION ${CMAKE_INSTALL_BINDIR})
> +install (PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/fw_setenv DESTINATION ${CMAKE_INSTALL_BINDIR})
>   
>   # Handle pkg-config files
>   set(prefix      ${CMAKE_INSTALL_PREFIX})
> diff --git a/src/fw_setenv.c b/src/fw_setenv.c
> deleted file mode 100644
> index e77e374..0000000
> --- a/src/fw_setenv.c
> +++ /dev/null
> @@ -1,8 +0,0 @@
> -/*
> - * (C) Copyright 2019
> - * Stefano Babic, DENX Software Engineering, sbabic@denx.de.
> - *
> - * SPDX-License-Identifier:     LGPL-2.1-or-later
> - */
> -
> -#include "fw_printenv.c"

Best regards,
Stefano Babic
Michael Heimpold March 2, 2023, 10:04 p.m. UTC | #2 
Hi Stefano,

Zitat von Stefano Babic <sbabic@denx.de>:

> Hi Michael,
>
> On 01.03.23 22:07, Michael Heimpold wrote:
>> Instead of compiling the same source file twice, just
>> add a symlink at install time. This is how the "original"
>> tools also worked.
>> It also reduces the storage footprint.
>>
>> Signed-off-by: Michael Heimpold <mhei@heimpold.de>
>> ---
>>
>> Please take this as RFC: the original tool installed a symlink as  
>> fw_setenv and
>> this tool seems to want to mimic the behavior - or is at least prepared for.
>>
>> However, it just does not do it - and I don't understand why.
>> Maybe I missed something - or there just wasn't yet time to  
>> implement it that
>> way?
>
> I let first to have two different tools in case new features are  
> asked, and while fw_printenv() will remain simple, fw_setenv could  
> be complex, like signing the environment, and so on. That means, I  
> decided first for two different tools, and then this was forgotten.

ok - thanks for the history and clarification.

>
> The tools are quite small, the reduced footprint is today negligible.
>

I agree.

>>
>> I don't know - so this is my proposal to change it :-)
>
> Have you already tested with OE that it is installed as expected ?

I just did a quick "compile" test and verified that the package-split  
shows the
desired result (I just changed my current kirkstone-based workspace  
via bbappend to my test branch
at https://github.com/mhei/libubootenv/tree/oe-test so, don't be  
confused about versioning):

-snip-
mhei@kerker:/srv/devel/y/build/tmp/work/cortexa7t2hf-neon-poky-linux-gnueabi/libubootenv/0.3.2-r0/packages-split$ ls -laR  
libubootenv-bin/usr/bin
libubootenv-bin/usr/bin:
total 20
drwxr-xr-x 2 mhei mhei 4096 Mar  2 07:31 .
drwxr-xr-x 3 mhei mhei 4096 Mar  2 07:31 ..
-rwxr-xr-x 2 mhei mhei 9720 Mar  2 07:31 fw_printenv
lrwxrwxrwx 1 mhei mhei   11 Mar  2 07:31 fw_setenv -> fw_printenv
-snap-

LGTM so far.

As mentioned, I just wondered and took this as a nice cmake-learning  
curve for me :-)
If you prefer to keep the current approach, I'm fine with your decision.

Best regards,
Michael
diff mbox series

Patch

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 6abdeb4..de4162b 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -21,15 +21,14 @@  SET_TARGET_PROPERTIES(ubootenv PROPERTIES VERSION ${VERSION} SOVERSION ${SOVERSI
 ADD_LIBRARY(ubootenv_static STATIC ${libubootenv_SOURCES} ${include_HEADERS})
 SET_TARGET_PROPERTIES(ubootenv_static PROPERTIES OUTPUT_NAME ubootenv)
 add_executable(fw_printenv fw_printenv.c)
-add_executable(fw_setenv fw_setenv.c)
 target_link_libraries(ubootenv z)
 target_link_libraries(fw_printenv ubootenv)
-target_link_libraries(fw_setenv ubootenv)
+add_custom_target(fw_setenv ALL ${CMAKE_COMMAND} -E create_symlink fw_printenv fw_setenv)
 
 install (TARGETS ubootenv ubootenv_static DESTINATION ${CMAKE_INSTALL_LIBDIR})
 install (FILES libuboot.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})
 install (TARGETS fw_printenv DESTINATION ${CMAKE_INSTALL_BINDIR})
-install (TARGETS fw_setenv DESTINATION ${CMAKE_INSTALL_BINDIR})
+install (PROGRAMS ${CMAKE_CURRENT_BINARY_DIR}/fw_setenv DESTINATION ${CMAKE_INSTALL_BINDIR})
 
 # Handle pkg-config files
 set(prefix      ${CMAKE_INSTALL_PREFIX})
diff --git a/src/fw_setenv.c b/src/fw_setenv.c
deleted file mode 100644
index e77e374..0000000
--- a/src/fw_setenv.c
+++ /dev/null
@@ -1,8 +0,0 @@ 
-/*
- * (C) Copyright 2019
- * Stefano Babic, DENX Software Engineering, sbabic@denx.de.
- *
- * SPDX-License-Identifier:     LGPL-2.1-or-later
- */
-
-#include "fw_printenv.c"