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

Adding a sub-dependency as explicit dependency wreaks havoc #1243

Open
jimklimov opened this issue Apr 2, 2020 · 3 comments
Open

Adding a sub-dependency as explicit dependency wreaks havoc #1243

jimklimov opened this issue Apr 2, 2020 · 3 comments

Comments

@jimklimov
Copy link
Member

Our colleague had a change to their project.xml file like this recently:

    <use project = "tntdb"
        repository = "https://github.com/42ity/tntdb.git"
        test = "tntdb::Date::gmtime"
        builddir = "tntdb">
        <use project = "cxxtools"
            header = "cxxtools/allocator.h"
            test = "cxxtools::Utf8Codec::Utf8Codec"
            repository = "https://github.com/42ity/cxxtools.git" />
    </use>

+    <use project = "cxxtools"
+        header = "cxxtools/allocator.h"
+        repository = "https://github.com/42ity/cxxtools.git" />

so the same dependency cxxtool as desired just above (as part of tntdb) was declared later than it was under tntdb; not sure if it matters much that the definition was different - e.g. here without a test attribute.

As a result, the regenerated project files liked the top-level definition over the sub-dependency, shuffling the order of references in all the Makefiles:

-    ${cxxtools_CFLAGS} \
    ${tntdb_CFLAGS} \
+    ${cxxtools_CFLAGS} \

... and packaging... and worse - in git checkouts and dependency pre-building: in the latter case, tntdb could not be built without cxxtools pre-built and installed, so the component's CI-from-scratch failed.

In the configure.ac changes, it is seen that the test attribute was no longer defined (so the top-level new definition did not "overlay" the one mentioned earlier in the project.xml file as a sub-dependency):

-      AC_MSG_NOTICE([Package cxxtools not found; falling back to defined compilability tests])
...
-      AC_CHECK_LIB([cxxtools], [cxxtools::Utf8Codec::Utf8Codec],

+     AC_MSG_WARN([Attribute 'test' is not set on zproject dependency 'cxxtools' - please ensure it is set])

They are checking if the other order of listing the <use> tags in project.xml fixes it (IIRC it should) as a workaround, but either way it is a bug, or two:

  • A big one to forget the sub-dependency completely.
  • Arguably a minor one to fully overwrite the definition of the (sub-)dependency - in fact we use such overwrites to e.g. customize the repo URL and so prefer our product-tuned forks over community upstreams from known_projects.xml (in view of this, gotta check if other attributes survive) effectively listing the same used project several times at same top level, and later consumers of same dependencies, e.g. many dep-trees rooted at ZMQ stack, automatically use the forks we need. As far as potential bugginess is concerned here, it would be reasonable to overlay the top-level's definition, if present, by attributes of a sub-dependency definition, if present (like cxxtools mentioned needed by tntdb above - re-use top-level attrs and add/replace customized ones), in the scope of such subdependency (remember the top-level cxxtools definition when parsing tntdb is done).
@sappo
Copy link
Member

sappo commented Apr 3, 2020

I'll have some time around easter to look into this.

@sappo
Copy link
Member

sappo commented Apr 10, 2020

Hi @jimklimov, I looked into this and figured out what is happening.

In case an implied use (in other word a use nested inside another use) is redefined on top level it is ignored!
https://github.com/zeromq/zproject/blob/master/zproject_projects.gsl#L134

IMO this is a sane behavior. Why would you define the same dependency in the project.xml twice anyway?

We might want to print a warning though to make this behavior visible to the user!

sappo added a commit to sappo/zproject that referenced this issue Apr 10, 2020
Solution: Make the user aware of a possible miss-configuration.

Relates to zeromq#1243
@sphaero
Copy link
Contributor

sphaero commented Feb 3, 2021

Can this be closed?

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

No branches or pull requests

3 participants