All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Herbert <marc.herbert@linux.intel.com>
To: Dave Jiang <dave.jiang@intel.com>,
	Alison Schofield <alison.schofield@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	nvdimm@lists.linux.dev, linux-cxl@vger.kernel.org
Subject: Re: [NDCTL PATCH v6 4/4] cxl/test: Add test for cxl features device
Date: Thu, 22 May 2025 11:25:04 -0700	[thread overview]
Message-ID: <acfac209-430b-486a-b468-3b98575b8b12@linux.intel.com> (raw)
In-Reply-To: <fc21a089-efa0-41b4-bc3e-70f6e7423719@intel.com>



>>>>>  
>>>>> +uuid_dep = dependency('uuid', required: false)
>>>>> +if get_option('fwctl').enabled() and uuid_dep.found()
>>>>> +  fwctl = executable('fwctl', 'fwctl.c',
>>>>> +    dependencies : libcxl_deps,
>>>>> +    include_directories : root_inc,
>>>>> +  )
>>>>> +  cxl_features = find_program('cxl-features.sh')
>>>>> +  tests += [
>>>>> +    [ 'cxl-features.sh',        cxl_features,       'cxl'   ],
>>>>> +  ]
>>>>> +endif
>>>>
>>>> Is the fwctl feature enabled fuss still needed now that the UAPI headers
>>>> are vendored locally?  Seems the test will quickly SKIP if fwctl dev not
>>>> found. I kind of like the idea of seeing a 'SKIP' and knowing the test
>>>> didn't run than seeing nothing at all in the test output.
>>>
>>> This gives the option to disable fwctl if needed. Also there is a libuuid dependency since we use uuid lib calls. 
>>
>> Is it possible to go anywhere without uuid?
>>
>> [snip]
>>
>> It looks like a hard requirement to me.
>>
>> UUIDs are rarely ever "optional". It could be required only
>> by some optional tool but that does not seem to be the case here
> 
> Apparently it's not an requirement for kernel build. As my feature user header caused issues due to lack of libuuid package.

I think I wasn't clear, let me be more explicit: I think you should drop
all the code with "uuid_dep" above. It would not just simplify the code
(uuid_dep.found() is always true), but it would also get rid of
some (unfounded) reader's anxiety about an (im)possible system
configuration where fwctl is not tested without even printing a "SKIP".

It never hurts to express dependencies, but I think it's also bad to
look like you can opt out when you cannot.

      reply	other threads:[~2025-05-22 18:25 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-09 16:39 [NDCTL PATCH v6 0/4] ndctl: Add support and test for CXL Features support Dave Jiang
2025-05-09 16:39 ` [NDCTL PATCH v6 1/4] cxl: Add cxl_bus_get_by_provider() Dave Jiang
2025-05-09 16:39 ` [NDCTL PATCH v6 2/4] cxl: Enumerate major/minor of FWCTL char device Dave Jiang
2025-05-09 16:39 ` [NDCTL PATCH v6 3/4] ndctl: Add features.h from kernel UAPI Dave Jiang
2025-05-09 16:39 ` [NDCTL PATCH v6 4/4] cxl/test: Add test for cxl features device Dave Jiang
2025-05-15 18:24   ` Alison Schofield
2025-05-15 18:28     ` Dave Jiang
2025-05-22 17:37       ` Marc Herbert
2025-05-22 17:46         ` Dave Jiang
2025-05-22 18:25           ` Marc Herbert [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=acfac209-430b-486a-b468-3b98575b8b12@linux.intel.com \
    --to=marc.herbert@linux.intel.com \
    --cc=alison.schofield@intel.com \
    --cc=dan.j.williams@intel.com \
    --cc=dave.jiang@intel.com \
    --cc=linux-cxl@vger.kernel.org \
    --cc=nvdimm@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.