Make thin_migrate tool optional by krnowak · Pull Request #1 · device-mapper-utils/thin-provisioning-tools

krnowak · 2025-11-20T12:34:15Z

Please consider making the aforementioned tool optional, but still enabled by default. The tool pulls in a dependency on libclang through devicemapper crate and it would be nice to skip it if possible.

In Flatcar we kept using version 1.0.10 to avoid building clang just for this one package. If possible, we would still like to keep it out. I came up with this patch, because our upstream (Gentoo) dropped this rather old version of the package.

Currently, the thin_migrate tool is behind the "migrate" feature, and installation of the symlink is can be disabled with the non-empty THIN_MIGRATE_EXCLUDE variable in Makefile.

mingnus

Thanks for the patch. I had considered this cleanup when modifying Makefile previously, but it ended up slipping through. Appreciate you addressing it.

mingnus · 2025-11-24T14:22:08Z

Makefile

-	ln -s -f pdata_tools $(BINDIR)/era_dump
-	ln -s -f pdata_tools $(BINDIR)/era_invalidate
-	ln -s -f pdata_tools $(BINDIR)/era_restore
+	for tool in $(TOOLS); do ln -s -f pdata_tools $(BINDIR)/$${tool}; done


Would you mind using the foreach function instead for better logging and portability?

$(foreach tool, $(TOOLS), ln -s -f pdata_tools $(BINDIR)/$(tool); $(NEWLINE))

Also define the NEWLINE variable in Makefile to separate the lines:

define NEWLINE endef

mingnus · 2025-11-24T14:23:29Z

Makefile

-	$(INSTALL_DATA) man8/era_restore.8 $(MANPATH)/man8
-	$(INSTALL_DATA) man8/era_invalidate.8 $(MANPATH)/man8
-	$(INSTALL_DATA) man8/thin_trim.8 $(MANPATH)/man8
+	for tool in $(TOOLS); do $(INSTALL_DATA) man8/$${tool}.8 $(MANPATH)/man8; done


Similarly,

$(foreach tool,$(TOOLS), $(INSTALL_DATA) man8/$(tool).8 $(MANPATH)/man8; $(NEWLINE))

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

krnowak · 2025-11-24T15:39:12Z

Thanks for the review. Replaced the bash loop with make loop. I also added a slightly-unrelated commit fixing a warning in txt2man that was showing up for every generated man-page - I must have updated something in the meantime, because I haven't seen it before.

mingnus

Review provided. I'm wondering why clang is avoided. Is that due to build errors or some other constraint?

mingnus · 2025-11-24T16:03:56Z

Cargo.toml

 thinp = { path = ".", features = ["devtools"] }

 [features]
+default = ["migrate"]


How about renaming the feature to thin_migrate for clarity?

mingnus · 2025-11-24T17:41:53Z

Makefile

 	era_invalidate \
 	era_restore

+ifeq ($(THIN_MIGRATE_EXCLUDE),)


Would you consider naming this DISABLE_THIN_MIGRATE to align with how the configure script handles naming?

Also, the flag should be propagate to the build command as well:

CARGO_FLAGS:=$(if $(DISABLE_THIN_MIGRATE),--no-default-features) $(PDATA_TOOLS): $(V) cargo build --release $(CARGO_FLAGS)

Given that users may run cargo build and make install separately, the feature availability check should be performed when the binary exists:

HAS_THIN_MIGRATE:=$(shell if [ -f $(PDATA_TOOLS) ]; then \ $(PDATA_TOOLS) 2>&1 | grep -q thin_migrate && echo 1; \ else \ $(if $(DISABLE_THIN_MIGRATE), true, echo 1); \ fi)

Finally, enable thin_migrate installation accordingly:

TOOLS:=\ ... era_restore \ $(if $(HAS_THIN_MIGRATE),thin_migrate)

(Both $(if and ifeq could be acceptable. I'm using $(if to make it consistent with the previous section)

That means, the DISABLE_THIN_MIGRATE flag doesn't affect the installation if the binary was built with thin_migrate enabled, or vise versa.

Would you consider naming this DISABLE_THIN_MIGRATE to align with how the configure script handles naming?

Sure, sounds good.

Also, the flag should be propagate to the build command as well:

CARGO_FLAGS:=$(if $(DISABLE_THIN_MIGRATE),--no-default-features) $(PDATA_TOOLS): $(V) cargo build --release $(CARGO_FLAGS)

Alright.

Given that users may run cargo build and make install separately, the feature availability check should be performed when the binary exists:

HAS_THIN_MIGRATE:=$(shell if [ -f $(PDATA_TOOLS) ]; then \ $(PDATA_TOOLS) 2>&1 | grep -q thin_migrate && echo 1; \ else \ $(if $(DISABLE_THIN_MIGRATE), true, echo 1); \ fi)

We are cross-compiling thin-provisioning-tools, so running $(PDATA_TOOLS) to check if migrate appears in the output won't work for us. Also, I'd print a warning about DISABLE_THIN_MIGRATE being ignored if the binary already exists. How about the following?

HAS_THIN_MIGRATE:=$(shell if [ -f $(PDATA_TOOLS).d ]; then \ grep -qF thin-migrate.rs $(PDATA_TOOLS).d && echo 1; \ else \ $(if $(DISABLE_THIN_MIGRATE), true, echo 2); \ fi) ifeq ($(HAS_THIN_MIGRATE),2) ifneq ($(DISABLE_THIN_MIGRATE),) $(warning DISABLE_THIN_MIGRATE variable is ignored, the pdata_tools binary exists and it has thin_migrate tool built in) endif endif

The $(PDATA_TOOLS).d seems to be some dependency file that lists all the source files that were used to build the binary. So if there is thin_migrate.rs listed, then it means that thin_migrate tool was built.

Finally, enable thin_migrate installation accordingly:

TOOLS:=\ ... era_restore \ $(if $(HAS_THIN_MIGRATE),thin_migrate)

(Both $(if and ifeq could be acceptable. I'm using $(if to make it consistent with the previous section)

That means, the DISABLE_THIN_MIGRATE flag doesn't affect the installation if the binary was built with thin_migrate enabled, or vise versa.

mingnus · 2025-11-25T10:21:05Z

bin/txt2man

 	sub(/^ +$/,"")
 }
-/^[:space:]*[[:upper:][:digit:]]+[[:upper:][:space:][:digit:][:punct:]]+$/ {
+/^[[:space:]]*[[:upper:][:digit:]]+[[:upper:][:space:][:digit:][:punct:]]+$/ {


The fix looks reasonable to me. However, I cannot reproduce the warning using gawk 5.3.0. Could you please let me know which version of gawk is reporting this warning? Also, would you mind reporting this issue to the txt2man upstream as well?

I'm using gawk 5.3.2. Also, this seems to be already reported: mvertes/txt2man#41 and my fix is probably not correct, see upstream commit with the fix.

I think I'll revert this commit for now.

Seems like the upstream commit was reverted too, so no fix yet. Probably a regression in gawk.

mingnus

Review the second commit

krnowak · 2025-11-25T14:55:31Z

Review provided. I'm wondering why clang is avoided. Is that due to build errors or some other constraint?

Just trying to avoid pulling in clang into Flatcar's SDK solely for the thin_migrate tool, which we didn't have so far, so seems like our users don't need it.

The tool pulls in, indirectly through the devicemapper crate, a dependency on libclang. Make it possible to skip the tool to avoid the dependency, but keep it enabled by default. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

krnowak · 2025-11-25T16:07:41Z

Updated.

mingnus · 2025-11-25T16:09:13Z

My concern is that disabling a high-level feature like thin_migrate doesn't solve the dependency issue for the whole project, as the bindgen & clang dependency is indirect (introduced by devicemapper-rs), and the devicemapper library may be used by other tools in the future.

If the goal is to provide a way to build without bindgen & clang, I believe a better approach would be to provide a minimal devicemapper wrapper with pre-generated bindings, thus eliminating the need for bindgen and clang at compile time. A feature flag will be used to toggle between the default devicemapper-rs and the minimal wrapper implementations.

What are your thoughts on this approach?

chewi · 2025-11-25T16:30:20Z

I did consider something like that myself, but I ruled it out because it wouldn't have been feasible for Flatcar to do it by itself, and then we saw that it only affected thin_migrate anyway. If you're happy to handle this upstream, that would be ideal. I don't think there would be any compatibility issues as the headers are bundled with devicemapper-rs.

mingnus · 2025-11-26T02:16:53Z

Understood. I will start investigating the necessary changes, but expect this to take some time. I'll provide updates here.

mingnus · 2025-12-19T13:25:27Z

@krnowak I'm drafting the fix. Since the Cargo features are strict additive, we'll have to specify the --no-default-features option to avoid compiling the default devicemapper-rs dependency, e.g.,

# use the internal devicemapper implementation
cargo build --no-default-features --features internal-dm

Does this necessity align with how you plan to integrate the crate?

chewi · 2025-12-19T13:48:48Z

@krnowak is away at the moment, but I've checked Gentoo's cargo.eclass, and it allows for conditionally enabling features as well as passing --no-default-features, so please go ahead. Thank you!

mingnus reviewed Nov 24, 2025

View reviewed changes

[build] Simplify installation of symlinks and manpages

7d89351

Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

krnowak force-pushed the krnowak/optional-migrate branch from fd3d574 to d560748 Compare November 24, 2025 15:37

mingnus reviewed Nov 24, 2025

View reviewed changes

mingnus reviewed Nov 25, 2025

View reviewed changes

[all] Make thin_migrate tool optional

32daf1b

The tool pulls in, indirectly through the devicemapper crate, a dependency on libclang. Make it possible to skip the tool to avoid the dependency, but keep it enabled by default. Signed-off-by: Krzesimir Nowak <knowak@microsoft.com>

krnowak force-pushed the krnowak/optional-migrate branch from d560748 to 32daf1b Compare November 25, 2025 16:07

Comments

Conversation

krnowak commented Nov 20, 2025

Uh oh!

mingnus left a comment

Choose a reason for hiding this comment

Uh oh!

mingnus Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

mingnus Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

krnowak commented Nov 24, 2025

Uh oh!

mingnus left a comment

Choose a reason for hiding this comment

Uh oh!

mingnus Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

mingnus Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krnowak Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingnus Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

krnowak Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

krnowak Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

krnowak Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

mingnus left a comment

Choose a reason for hiding this comment

Uh oh!

krnowak commented Nov 25, 2025

Uh oh!

krnowak commented Nov 25, 2025

Uh oh!

mingnus commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chewi commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mingnus commented Nov 26, 2025

Uh oh!

mingnus commented Dec 19, 2025

Uh oh!

chewi commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mingnus Nov 24, 2025 •

edited

Loading

krnowak Nov 25, 2025 •

edited

Loading

mingnus commented Nov 25, 2025 •

edited

Loading

chewi commented Nov 25, 2025 •

edited

Loading