Re: [RFC PATCH v3 1/3] scripts: add kconfirm

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3067 bytes --]

On 5/17/26 02:28, Miguel Ojeda wrote:
> On Sat, May 16, 2026 at 11:54 PM Julian Braha <julianbraha@gmail.com> wrote:
>>
>> +CARGO          = cargo
> 
> Question to Kbuild: would it hurt to hardcore `--offline` here?
> 
> If someone within Make actually ever needs Cargo to fetch something,
> then they should be very explicit about it (in which case we could
> have another variable etc.).
> 
>> -                 rust/libpin_init_internal.so rust/libpin_init_internal.dylib
>> +                 rust/libpin_init_internal.so rust/libpin_init_internal.dylib \
> 
> Spurious change?
> 
>> +$(TARGET):
>> +       $(CARGO) run --release --offline -p kconfirm-linux -- --linux-path $(srctree) --enable-arch $(SRCARCH) $(KCONFIRM_ARGS)
> 
> This probably does not work in `O=` builds or in cases where the
> `srctree` is read-only (please see my other reply on the docs patch).
> 
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +use crate::AnalysisArgs;
> 
> This appears to use a quite different, custom Rust style. If this is
> going to be developed in-tree, then we should do our best to follow
> the guidelines:
> 
>   https://docs.kernel.org/rust/coding-guidelines.html
> 
> For instance, a few key points are not followed here:
> 
>   - Public items are not documented. When they are, they seem to use
> comments instead of actual docs. Markdown is not used either.
> 
>   - No examples, doctests or tests.
> 
>   - No `// SAFETY` comments for unsafe code.
> 
>> +unsafe extern "C" {
>> +    fn curl_global_init(flags: c_long) -> CURLcode;
>> +
>> +    fn curl_easy_init() -> *mut CURL;
>> +
>> +    fn curl_easy_cleanup(handle: *mut CURL);
>> +
>> +    fn curl_easy_perform(handle: *mut CURL) -> CURLcode;
>> +
>> +    fn curl_easy_strerror(code: CURLcode) -> *const c_char;
>> +
>> +    fn curl_easy_setopt(handle: *mut CURL, option: CURLoption, ...) -> CURLcode;
>> +
>> +    fn curl_easy_getinfo(handle: *mut CURL, info: CURLINFO, ...) -> CURLcode;
>> +}
> 
> I like minimizing dependencies, but since we require vendored
> dependencies anyway, then it may be simpler to use a common
> ("standard") Rust dependency for these things. Then we can all agree
> on a particular one and use that when the same need arises.

Using a ton of vendored dependencies would make for unreviewable
patches.  It's also the kind of thing that gives distro packagers
headaches.

> In fact, it seems like FFI here and in the other file is the only
> source of `unsafe` code, no? We could perhaps even `forbid` it
> otherwise.

I don't want to pull in a huge library like clap.  A simple getopt_long
implementation could also be an option.

> If we truly want to minimize dependencies even if we have vendored
> ones, then we could call into the `curl` CLI instead, just like we
> e.g. call into `bindgen` instead of using its library.

I'm the one who suggested using FFI here and for command-line parsing.
The command-line interface would also work.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]