- Package must include LICENSE and should include README.md

%files          devel
%license LICENSE
%doc README.md
%{cargo_registry}/%{crate}-%{version_no_tilde}/

If not provided by the crate, please ask upstream to include the LICENSE within their crate.

 - devel should depend on libs + use pkgconfig in the auto-BR part:

BuildRequires:	cryptsetup-libs
BuildRequires:	cryptsetup-devel

 → 

%generate_buildrequires
%cargo_generate_buildrequires
echo 'pkgconfig(libcryptsetup)'


 - Package devel should also Requires pkgconfig(libcryptsetup) (When the package is installed, it is to be used in building a project thus requiring the libraries).


%package        devel
Summary:        %{summary}
BuildArch:      noarch
Requires:       pkgconfig(libcryptsetup)

Hi Robert-André, thanks for looking into this. I'm not entirely clear on all of the feedback here.

I checked and see that the LICENSE and README.md files are missing from the crate so that I can take care of upstream for sure.

With regards to the comment "devel should depend on libs + use pkgconfig in the auto-BR part" can you clarify a little bit? Do you mean add this line:

echo 'pkgconfig(libcryptsetup)'

and remove this line:

BuildRequires:	cryptsetup-devel

Will pkgconfig pull in the dependency on cryptsetup-devel? That's needed for the header file from which bindgen generates the bindings.

With regards to the comment "Package devel should also Requires pkgconfig(libcryptsetup) (When the package is installed, it is to be used in building a project thus requiring the libraries)", is this necessary so that applications that use the library pull in the proper runtime dependencies? Will this propagate to applications that pull in this library as a crate dependency?

Thanks for your help!

(In reply to John Baublitz from comment #2)
> Hi Robert-André, thanks for looking into this. I'm not entirely clear on all
> of the feedback here.
> 
> I checked and see that the LICENSE and README.md files are missing from the
> crate so that I can take care of upstream for sure.
> 
Yes please ask upstream.

> With regards to the comment "devel should depend on libs + use pkgconfig in
> the auto-BR part" can you clarify a little bit? Do you mean add this line:
> 
> echo 'pkgconfig(libcryptsetup)'
> 
> and remove this line:
> 
> BuildRequires:	cryptsetup-devel
> 
Remove:

BuildRequires:	cryptsetup-libs
BuildRequires:	cryptsetup-devel

Add:

echo 'pkgconfig(libcryptsetup)'

below:

%generate_buildrequires
%cargo_generate_buildrequires



> Will pkgconfig pull in the dependency on cryptsetup-devel? That's needed for
> the header file from which bindgen generates the bindings.
> 

Yes, pkgconfig(libcryptsetup) == cryptsetup-devel

> With regards to the comment "Package devel should also Requires
> pkgconfig(libcryptsetup) (When the package is installed, it is to be used in
> building a project thus requiring the libraries)", is this necessary so that
> applications that use the library pull in the proper runtime dependencies?
> 

Rust binary are statically linked, so the only time we need to install a crate is as a dependency for building a binary. So when you depend on your crate, it needs to pull the pkgconfig(libcryptsetup) to build the project.

> Will this propagate to applications that pull in this library as a crate
> dependency?

Only at the building stage, the final binary packaged won't need the dependency.

> Thanks for your help!

Hi Robert-André, this makes a lot more sense. Thank you for the clarification. I'll make the requested changes to the spec file as well. I just have one last question for future reference. The addition of this package is to support building a Rust application that takes advantage of libcryptsetup functionality. While it makes sense that the devel package for the bindings should require libcryptsetup-devel to be installed if this package is installed, should the runtime dependency on libcryptsetup be handled by the application I mentioned? I've dealt with Rust builds outside of Fedora packaging quite a lot and there's not really a good way to ensure through cargo that external dependencies are installed for bindings to execute properly and so the best you can usually do is check that a runtime application is installed in the application code because, like you mentioned, only static linking is supported at the crate level. However, with Fedora packaging there's a way to enforce non-Rust dependencies. Does that responsibility fall on the application RPM to enforce? I can't imagine there's a way to enforce this from a library as these RPMs seem to only be used in service of building a final application executable from the documents I've read.

(In reply to John Baublitz from comment #4)
> Hi Robert-André, this makes a lot more sense. Thank you for the
> clarification. I'll make the requested changes to the spec file as well. I
> just have one last question for future reference. The addition of this
> package is to support building a Rust application that takes advantage of
> libcryptsetup functionality. While it makes sense that the devel package for
> the bindings should require libcryptsetup-devel to be installed if this
> package is installed, should the runtime dependency on libcryptsetup be
> handled by the application I mentioned? I've dealt with Rust builds outside
> of Fedora packaging quite a lot and there's not really a good way to ensure
> through cargo that external dependencies are installed for bindings to
> execute properly and so the best you can usually do is check that a runtime
> application is installed in the application code because, like you
> mentioned, only static linking is supported at the crate level. However,
> with Fedora packaging there's a way to enforce non-Rust dependencies. Does
> that responsibility fall on the application RPM to enforce? I can't imagine
> there's a way to enforce this from a library as these RPMs seem to only be
> used in service of building a final application executable from the
> documents I've read.

That should be automatically handled, for example I have a crate rust-zstd-sys and the final binary RPM I build with it depends on libzstd.so.1()(64bit)

(In reply to John Baublitz from comment #0)
 
> The only failure was on 32 bit architectures which should not impact
> rawhide. I've filed a bug about this issue here:
> https://github.com/rust-lang/rust-bindgen/issues/1823

Rawhide package are built on all supported arches, even 32 bits one (i686 and armv7hl)

(In reply to Robert-André Mauchin 🐧 from comment #6)
> (In reply to John Baublitz from comment #0)
>  
> > The only failure was on 32 bit architectures which should not impact
> > rawhide. I've filed a bug about this issue here:
> > https://github.com/rust-lang/rust-bindgen/issues/1823
> 
> Rawhide package are built on all supported arches, even 32 bits one (i686
> and armv7hl)

I did some research and was able to make some code changes that satisfy the koji build requirements for 32 bit architectures with help from bindgen upstream.

The code that I used for the koji build will be merged shortly and the PR is here: https://github.com/stratis-storage/libcryptsetup-rs/pull/87

The koji build is here: https://koji.fedoraproject.org/koji/taskinfo?taskID=46530487

I'll upload the new source RPM and spec file for a second review once I merge the above PR and upload the new version to crates.io. Thanks!

Passing koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=46573145
Source RPM: https://download.copr.fedorainfracloud.org/results/jbaublitz/rust-libcryptsetup-rs-sys/fedora-rawhide-x86_64/01517705-rust-libcryptsetup-rs-sys/rust-libcryptsetup-rs-sys-0.1.4-1.fc33.src.rpm
Spec file: https://download.copr.fedorainfracloud.org/results/jbaublitz/rust-libcryptsetup-rs-sys/fedora-rawhide-x86_64/01517705-rust-libcryptsetup-rs-sys/rust-libcryptsetup-rs-sys.spec

This is ready for review again. Thanks!

Package approved. You still need to seek a sponsor: https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libcryptsetup-rs-sys