[Buildroot] [PATCH V3] Adding support for NVME utils

[Buildroot] [PATCH V3] Adding support for NVME utils

[Mamatha]

Add support for building NVME utility - a utility for interacting with
standard NVM Express (optimized PCI Express SSD interface) devices.

Signed-off-by: Mamatha <mamatha4@linux.vnet.ibm.com>
---
 package/Config.in      |    1 +
 package/nvme/Config.in |    6 ++++++
 package/nvme/nvme.mk   |   23 +++++++++++++++++++++++
 3 files changed, 30 insertions(+)
 create mode 100755 package/nvme/Config.in
 create mode 100755 package/nvme/nvme.mk

diff --git a/package/Config.in b/package/Config.in
index e0c2e2a..014debd 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -1399,6 +1399,7 @@ endif
 	source "package/openvmtools/Config.in"
 	source "package/polkit/Config.in"
 	source "package/powerpc-utils/Config.in"
+	source "package/nvme/Config.in"
 if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
 	source "package/procps-ng/Config.in"
 	source "package/psmisc/Config.in"
diff --git a/package/nvme/Config.in b/package/nvme/Config.in
new file mode 100755
index 0000000..a3c8ada
--- /dev/null
+++ b/package/nvme/Config.in
@@ -0,0 +1,6 @@
+config BR2_PACKAGE_NVME
+	bool "nvme"
+	help
+	  System utilities for standard NVME Express devices
+
+	  https://github.com/linux-nvme/nvme-cli
diff --git a/package/nvme/nvme.mk b/package/nvme/nvme.mk
new file mode 100755
index 0000000..43bec67
--- /dev/null
+++ b/package/nvme/nvme.mk
@@ -0,0 +1,23 @@
+################################################################################
+#
+# nvme
+#
+################################################################################
+
+NVME_VERSION = 0.1
+NVME_SITE = https://github.com/linux-nvme/nvme-cli.git
+NVME_SITE_METHOD = git
+NVME_LICENSE = GPLv2
+NVME_LICENSE_FILES = COPYING
+
+define NVME_BUILD_CMDS
+	$(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D) \
+		INCLUDEDIR="-I." all
+endef
+
+define NVME_INSTALL_TARGET_CMDS
+	$(INSTALL) -D -m 0755 $(@D)/nvme $(TARGET_DIR)/sbin/nvme
+endef
+
+$(eval $(generic-package))
+

[Buildroot] [PATCH V3] Adding support for NVME utils

[Thomas Petazzoni]

Dear Mamatha,

Thanks for your contribution!

On Tue, 12 Jan 2016 11:22:06 +0530, Mamatha wrote:
> Add support for building NVME utility - a utility for interacting with
> standard NVM Express (optimized PCI Express SSD interface) devices.

For all patches on packages, we prefer to have the title of the commit
comply with the following format:

	<package>: <description>

So in your case, it should be:

	nvme: new package

> Signed-off-by: Mamatha <mamatha4@linux.vnet.ibm.com>

Sorry if I'm not familiar with naming practices in your part of the
world, but we require a full name (i.e first name + last name). Is
Mamatha your full name ?

> ---
>  package/Config.in      |    1 +
>  package/nvme/Config.in |    6 ++++++
>  package/nvme/nvme.mk   |   23 +++++++++++++++++++++++
>  3 files changed, 30 insertions(+)
>  create mode 100755 package/nvme/Config.in
>  create mode 100755 package/nvme/nvme.mk
> 
> diff --git a/package/Config.in b/package/Config.in
> index e0c2e2a..014debd 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1399,6 +1399,7 @@ endif
>  	source "package/openvmtools/Config.in"
>  	source "package/polkit/Config.in"
>  	source "package/powerpc-utils/Config.in"
> +	source "package/nvme/Config.in"

You should respect alphabetic ordering here when including
nvme/Config.in. Also I believe this package better belongs to the
"Hardware handling" menu rather than "System tools".

>  if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
>  	source "package/procps-ng/Config.in"
>  	source "package/psmisc/Config.in"
> diff --git a/package/nvme/Config.in b/package/nvme/Config.in
> new file mode 100755
> index 0000000..a3c8ada
> --- /dev/null
> +++ b/package/nvme/Config.in
> @@ -0,0 +1,6 @@
> +config BR2_PACKAGE_NVME
> +	bool "nvme"
> +	help
> +	  System utilities for standard NVME Express devices
> +
> +	  https://github.com/linux-nvme/nvme-cli
> diff --git a/package/nvme/nvme.mk b/package/nvme/nvme.mk
> new file mode 100755
> index 0000000..43bec67
> --- /dev/null
> +++ b/package/nvme/nvme.mk
> @@ -0,0 +1,23 @@
> +################################################################################
> +#
> +# nvme
> +#
> +################################################################################
> +
> +NVME_VERSION = 0.1
> +NVME_SITE = https://github.com/linux-nvme/nvme-cli.git
> +NVME_SITE_METHOD = git

You should use the github helper. See also
https://buildroot.org/downloads/manual/manual.html#github-download-url.

> +NVME_LICENSE = GPLv2

The license is GPLv2+

> +NVME_LICENSE_FILES = COPYING
> +
> +define NVME_BUILD_CMDS
> +	$(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D) \
> +		INCLUDEDIR="-I." all

Why do you explicitly specify "all" here ? It is going to build the
documentation, while if you leave it out, it will only build the first
target, which is named "default" in nvme's Makefile, and it will only
build the nvme binary.

Also, when calling $(MAKE), please pass $(TARGET_MAKE_ENV) in the
environment:

	$(TARGET_MAKE_ENV) $(MAKE) ...

> +endef
> +
> +define NVME_INSTALL_TARGET_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/nvme $(TARGET_DIR)/sbin/nvme

Why you don't you use the install-bin target ? Like this:

	$(TARGET_MAKE_ENV) $(MAKE) -C (@D) DESTDIR=$(TARGET_DIR) install-bin

Also, could you add a hash file, as explained in
https://buildroot.org/downloads/manual/manual.html#adding-packages-hash ?

Could you rework your patch to take into account those comments and
send an updated version ?

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH V3] Adding support for NVME utils

[Mamatha Inamdar]

Hi Thomas,

Thanks for reviewing the patch.

I have worked on all your comments,facing some issue while building nvme.
Will check and send new version of patch soon.


On 01/13/2016 01:57 AM, Thomas Petazzoni wrote:
> Dear Mamatha,
>
> Thanks for your contribution!
>
> On Tue, 12 Jan 2016 11:22:06 +0530, Mamatha wrote:
>> Add support for building NVME utility - a utility for interacting with
>> standard NVM Express (optimized PCI Express SSD interface) devices.
> For all patches on packages, we prefer to have the title of the commit
> comply with the following format:
>
> 	<package>: <description>
>
> So in your case, it should be:
>
> 	nvme: new package
>
>> Signed-off-by: Mamatha <mamatha4@linux.vnet.ibm.com>
> Sorry if I'm not familiar with naming practices in your part of the
> world, but we require a full name (i.e first name + last name). Is
> Mamatha your full name ?
>
>> ---
>>   package/Config.in      |    1 +
>>   package/nvme/Config.in |    6 ++++++
>>   package/nvme/nvme.mk   |   23 +++++++++++++++++++++++
>>   3 files changed, 30 insertions(+)
>>   create mode 100755 package/nvme/Config.in
>>   create mode 100755 package/nvme/nvme.mk
>>
>> diff --git a/package/Config.in b/package/Config.in
>> index e0c2e2a..014debd 100644
>> --- a/package/Config.in
>> +++ b/package/Config.in
>> @@ -1399,6 +1399,7 @@ endif
>>   	source "package/openvmtools/Config.in"
>>   	source "package/polkit/Config.in"
>>   	source "package/powerpc-utils/Config.in"
>> +	source "package/nvme/Config.in"
> You should respect alphabetic ordering here when including
> nvme/Config.in. Also I believe this package better belongs to the
> "Hardware handling" menu rather than "System tools".
>
>>   if BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
>>   	source "package/procps-ng/Config.in"
>>   	source "package/psmisc/Config.in"
>> diff --git a/package/nvme/Config.in b/package/nvme/Config.in
>> new file mode 100755
>> index 0000000..a3c8ada
>> --- /dev/null
>> +++ b/package/nvme/Config.in
>> @@ -0,0 +1,6 @@
>> +config BR2_PACKAGE_NVME
>> +	bool "nvme"
>> +	help
>> +	  System utilities for standard NVME Express devices
>> +
>> +	  https://github.com/linux-nvme/nvme-cli
>> diff --git a/package/nvme/nvme.mk b/package/nvme/nvme.mk
>> new file mode 100755
>> index 0000000..43bec67
>> --- /dev/null
>> +++ b/package/nvme/nvme.mk
>> @@ -0,0 +1,23 @@
>> +################################################################################
>> +#
>> +# nvme
>> +#
>> +################################################################################
>> +
>> +NVME_VERSION = 0.1
>> +NVME_SITE = https://github.com/linux-nvme/nvme-cli.git
>> +NVME_SITE_METHOD = git
> You should use the github helper. See also
> https://buildroot.org/downloads/manual/manual.html#github-download-url.
>
>> +NVME_LICENSE = GPLv2
> The license is GPLv2+
>
>> +NVME_LICENSE_FILES = COPYING
>> +
>> +define NVME_BUILD_CMDS
>> +	$(MAKE) $(TARGET_CONFIGURE_OPTS) -C $(@D) \
>> +		INCLUDEDIR="-I." all
> Why do you explicitly specify "all" here ? It is going to build the
> documentation, while if you leave it out, it will only build the first
> target, which is named "default" in nvme's Makefile, and it will only
> build the nvme binary.
>
> Also, when calling $(MAKE), please pass $(TARGET_MAKE_ENV) in the
> environment:
>
> 	$(TARGET_MAKE_ENV) $(MAKE) ...
>
>> +endef
>> +
>> +define NVME_INSTALL_TARGET_CMDS
>> +	$(INSTALL) -D -m 0755 $(@D)/nvme $(TARGET_DIR)/sbin/nvme
> Why you don't you use the install-bin target ? Like this:
>
> 	$(TARGET_MAKE_ENV) $(MAKE) -C (@D) DESTDIR=$(TARGET_DIR) install-bin
>
> Also, could you add a hash file, as explained in
> https://buildroot.org/downloads/manual/manual.html#adding-packages-hash ?
>
> Could you rework your patch to take into account those comments and
> send an updated version ?
>
> Thanks!
>
> Thomas

[Buildroot] [PATCH V3] Adding support for NVME utils

[Thomas Petazzoni]

Mamatha,

On Wed, 13 Jan 2016 16:35:22 +0530, Mamatha Inamdar wrote:

> Thanks for reviewing the patch.

You're welcome, thanks for your contribution in the first place.

> I have worked on all your comments,facing some issue while building nvme.

Which issues?

> Will check and send new version of patch soon.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH V3] Adding support for NVME utils

[Thomas Petazzoni]

Hello,

Please don't reply to me directly, always keep the list in Cc.

On Thu, 14 Jan 2016 09:45:59 +0530, Mamatha Inamdar wrote:

> I am facing two issues here.
> 
> 1) NVME_VERSION = 0.1
>      If I update version as 0.1 and compile the build, It's unable to 
> build and following errors occur

You need to do:

NVME_VERSION = v0.2
NVME_SITE = $(call github,linux-nvme,nvme-cli,$(NVME_VERSION))

(and not define the NVME_SOURCE variable)

Note that I've changed the version from 0.1 to 0.2, since 0.2 is
available, so you should use this one.


> 2) Second issue is...If I use the install-bin target Like this:
> 
> $(TARGET_MAKE_ENV) $(MAKE) -C (@D) DESTDIR=$(TARGET_DIR) install-bin
> 
> I don't see nvme tool installed in sbin/bin, but it works for the 
> following install command

Because you also need to set PREFIX=/usr otherwise it gets installed
in /usr/local.

A few other things:

 *) You need to patch the Makefile to remove -m64 and -Werror otherwise
    it won't build

 *) You need to pass $(TARGET_CONFIGURE_OPTS) on the left hand side of
    $(MAKE) rather than the right hand side so that the CFLAGS from
    $(TARGET_CONFIGURE_OPTS) don't override the ones defined in the
    Makefile. Without this, -std=gnu99 is no longer passed in the CFLAGS,
    and the package fails to build.

 *) INCLUDEDIR="-I." becomes useless.

 *) You need to pass LIBUDEV=1 to disable using the udev library. If
    you don't do this, the Makefile looks on the host machine if libudev
    is installed to determine whether it should enable udev support or
    not, which is completely wrong since we're cross-compiling.
    Alternatively, you could optionally add libudev support if you
    want, but that's not mandatory.

 *) Since the upstream project is named nvme-cli, I think the Buildroot
    package should also be named nvme-cli.

All in all, I had a .mk file that looks like this:

NVME_VERSION = v0.2
NVME_SITE = $(call github,linux-nvme,nvme-cli,$(NVME_VERSION))
NVME_LICENSE = GPLv2
NVME_LICENSE_FILES = COPYING

# LIBUDEV=1 indicates that we don't want udev support
define NVME_BUILD_CMDS
	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) \
		LIBUDEV=1 -C $(@D)
endef

define NVME_INSTALL_TARGET_CMDS
	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) \
		PREFIX=/usr install-bin
endef

$(eval $(generic-package))

Hope this helps,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH V3] Adding support for NVME utils

[Mamatha Inamdar]

Thanks for the detailed explanation,
updated your comments and sent new version of Patch.

Thanks
Mamatha

On 01/14/2016 02:34 PM, Thomas Petazzoni wrote:
> Hello,
>
> Please don't reply to me directly, always keep the list in Cc.
>
> On Thu, 14 Jan 2016 09:45:59 +0530, Mamatha Inamdar wrote:
>
>> I am facing two issues here.
>>
>> 1) NVME_VERSION = 0.1
>>       If I update version as 0.1 and compile the build, It's unable to
>> build and following errors occur
> You need to do:
>
> NVME_VERSION = v0.2
> NVME_SITE = $(call github,linux-nvme,nvme-cli,$(NVME_VERSION))
>
> (and not define the NVME_SOURCE variable)
>
> Note that I've changed the version from 0.1 to 0.2, since 0.2 is
> available, so you should use this one.
>
>
>> 2) Second issue is...If I use the install-bin target Like this:
>>
>> $(TARGET_MAKE_ENV) $(MAKE) -C (@D) DESTDIR=$(TARGET_DIR) install-bin
>>
>> I don't see nvme tool installed in sbin/bin, but it works for the
>> following install command
> Because you also need to set PREFIX=/usr otherwise it gets installed
> in /usr/local.
>
> A few other things:
>
>   *) You need to patch the Makefile to remove -m64 and -Werror otherwise
>      it won't build
>
>   *) You need to pass $(TARGET_CONFIGURE_OPTS) on the left hand side of
>      $(MAKE) rather than the right hand side so that the CFLAGS from
>      $(TARGET_CONFIGURE_OPTS) don't override the ones defined in the
>      Makefile. Without this, -std=gnu99 is no longer passed in the CFLAGS,
>      and the package fails to build.
>
>   *) INCLUDEDIR="-I." becomes useless.
>
>   *) You need to pass LIBUDEV=1 to disable using the udev library. If
>      you don't do this, the Makefile looks on the host machine if libudev
>      is installed to determine whether it should enable udev support or
>      not, which is completely wrong since we're cross-compiling.
>      Alternatively, you could optionally add libudev support if you
>      want, but that's not mandatory.
>
>   *) Since the upstream project is named nvme-cli, I think the Buildroot
>      package should also be named nvme-cli.
>
> All in all, I had a .mk file that looks like this:
>
> NVME_VERSION = v0.2
> NVME_SITE = $(call github,linux-nvme,nvme-cli,$(NVME_VERSION))
> NVME_LICENSE = GPLv2
> NVME_LICENSE_FILES = COPYING
>
> # LIBUDEV=1 indicates that we don't want udev support
> define NVME_BUILD_CMDS
> 	$(TARGET_MAKE_ENV) $(TARGET_CONFIGURE_OPTS) $(MAKE) \
> 		LIBUDEV=1 -C $(@D)
> endef
>
> define NVME_INSTALL_TARGET_CMDS
> 	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) \
> 		PREFIX=/usr install-bin
> endef
>
> $(eval $(generic-package))
>
> Hope this helps,
>
> Thomas