Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove cycles from CMake dependency graph #2820

Conversation

bracketttc
Copy link

Attempts to generate a Ninja build from the existing CMake structure failed complaining about phony cycles related to the use of add_custom_target and placing the primary output file in the DEPENDS option.

Attempts to generate a Ninja build from the existing CMake structure
failed complaining about phony cycles related to the use of
`add_custom_target` and placing the primary output file in the
`DEPENDS` option.
@@ -434,17 +434,17 @@ function(add_tarball targetname namever treeish)
set(distname ${tarname}.bz2)
set(docname ${namever}-doc.${distfmt})

add_custom_target(${docname}
DEPENDS man apidoc
file(GLOB man_pages docs/man/*.[1-8])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for this? AFAICS the tarball gets updated on man page changes as it is, through the "man" target.

@pmatilai
Copy link
Member

pmatilai commented Jan 9, 2024

Right, I remember encountering this pattern somewhere. Always looked like one of those cmake WTFs for me, which is probably why I tried to find some other way to do it. And as such, it was probably always for the worse.

I'll accept changing this for correctness, but note that we officially only support "make" based builds as per INSTALL.

Copy link
Member

@pmatilai pmatilai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came this close to merging, but testing made me remember why it's done the way it is: we want the tarballs always recreated on "dist". With this patch, that no longer happens.

Of course loops can't be right either, but I can't apply this as-is.

@pmatilai pmatilai self-assigned this Jan 17, 2024
@pmatilai
Copy link
Member

Just FWIW, https://gitlab.kitware.com/cmake/cmake/-/issues/22852 describes this very case.

@bracketttc
Copy link
Author

we want the tarballs always recreated on "dist".

Okay, I get that.

@pmatilai
Copy link
Member

pmatilai commented Feb 13, 2024

Solved a bit differently by just dropping the bogus BYPRODUCTS directives: #2900

Thanks for reporting, and the patch even if it didn't get used as-is!

@pmatilai pmatilai closed this Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants