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

Fix Dub dependencies #10045

Merged
merged 2 commits into from
Mar 23, 2022
Merged

Fix Dub dependencies #10045

merged 2 commits into from
Mar 23, 2022

Conversation

rtbo
Copy link
Contributor

@rtbo rtbo commented Mar 1, 2022

This is nearly a complete rewrite of Dub dependencies that were broken except for the most simple cases.

My test case is a simple executable depending on vibe-d:http. Vibe-d is complex and has a many dependencies.
I tested with success on Linux and Windows (to figure out the right order of static libraries took some time but is now OK).

The non-compatibility with Dub package having sub-dependencies is especially annoying and is the main motivation for me to write this PR.
My meson file before:

vibed_deps = [
    dependency('stdx-allocator',
        version: '2.77.5',
        method: 'dub',
    ),
    dependency('eventcore',
        version: '0.9.20',
        method: 'dub',
    ),
    dependency('vibe-core',
        version: '1.22.0',
        method: 'dub',
    ),
    dependency('diet-ng',
        version: '1.8.0',
        method: 'dub',
    ),
    dependency('taggedalgebraic',
        version: '0.11.22',
        method: 'dub',
    ),
    dependency('vibe-d:utils',
        version: '0.9.4',
        method: 'dub',
    ),
    dependency('vibe-d:crypto',
        version: '0.9.4',
        method: 'dub',
    ),
    dependency('vibe-d:stream',
        version: '0.9.4',
        method: 'dub',
    ),
    dependency('vibe-d:textfilter',
        version: '0.9.4',
        method: 'dub',
    ),
    dependency('vibe-d:data',
        version: '0.9.4',
        method: 'dub',
    ),
    dependency('vibe-d:tls',
        version: '0.9.4',
        method: 'dub',
    ),
    dependency('vibe-d:inet',
        version: '0.9.4',
        method: 'dub',
    ),
    dependency('vibe-d:http',
        version: '0.9.4',
        method: 'dub',
    ),
]
if host_machine.system() == 'windows'
    vibed_deps += declare_dependency(
        link_args: [
            join_paths(meson.source_root(), 'libs/windows-x64/libcrypto.lib'),
            join_paths(meson.source_root(), 'libs/windows-x64/libssl.lib'),
            'user32.lib'
        ],
    )
endif

My meson file now:

vibed_deps = [
    dependency('vibe-d:http',
        version: '0.9.4',
        method: 'dub',
    ),
]

@rtbo rtbo requested a review from jpakkane as a code owner March 1, 2022 21:51
mesonbuild/dependencies/dub.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 1, 2022

Codecov Report

Merging #10045 (f7a9a8c) into master (cec7d49) will decrease coverage by 4.88%.
The diff coverage is n/a.

❗ Current head f7a9a8c differs from pull request most recent head 1d54733. Consider uploading reports for the commit 1d54733 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10045      +/-   ##
==========================================
- Coverage   66.22%   61.34%   -4.89%     
==========================================
  Files         404      202     -202     
  Lines       86216    43035   -43181     
  Branches    19021     9438    -9583     
==========================================
- Hits        57100    26399   -30701     
+ Misses      24716    14533   -10183     
+ Partials     4400     2103    -2297     
Impacted Files Coverage Δ
dependencies/dub.py 8.05% <0.00%> (-3.98%) ⬇️
modules/python.py 68.65% <0.00%> (-0.07%) ⬇️
mesonbuild/dependencies/dub.py
mesonbuild/modules/python.py
mesonbuild/dependencies/factory.py
mesonbuild/linkers/linkers.py
mesonbuild/compilers/mixins/ccrx.py
mesonbuild/interpreter/primitives/string.py
mesonbuild/interpreter/primitives/dict.py
mesonbuild/backend/vs2013backend.py
... and 194 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44104e8...1d54733. Read the comment docs.

@eli-schwartz
Copy link
Member

Commits 3 and 4 should probably be rolled as fixups into the first commit.

@rtbo
Copy link
Contributor Author

rtbo commented Mar 2, 2022

Commits 3 and 4 should probably be rolled as fixups into the first commit.

I agree but I'm not sure how to do this with commit 2 sitting between them. In general I'd expect that you would squash everything into a single commit before merge.

mesonbuild/dependencies/dub.py Outdated Show resolved Hide resolved
@rtbo
Copy link
Contributor Author

rtbo commented Mar 4, 2022

If a dependency is found but not with the same build-type, a warning is shown, but the dependency is accepted nevertheless.

Here is what the user sees:
image

@rtbo rtbo force-pushed the fix_dub_deps branch 5 times, most recently from 67210da to a2d2474 Compare March 5, 2022 09:31
@rtbo
Copy link
Contributor Author

rtbo commented Mar 5, 2022

Each time I bring modification to ci/ciimage/opensuse/install.sh, there are weird errors in the Arch and Opensuse test images.
It is systematically the same failure in vala/14 target glib version and gresources:

[1/6] /usr/bin/glib-compile-resources '../test cases/vala/14 target glib version and gresources/gres/test-resources.xml' --sourcedir '../test cases/vala/14 target glib version and gresources/gres/.' --sourcedir '../test cases/vala/14 target glib version and gresources/gres' --internal --generate --target gres/testui.c --dependency-file gres/testui.c.d
FAILED: gres/testui.c 
/usr/bin/glib-compile-resources '../test cases/vala/14 target glib version and gresources/gres/test-resources.xml' --sourcedir '../test cases/vala/14 target glib version and gresources/gres/.' --sourcedir '../test cases/vala/14 target glib version and gresources/gres' --internal --generate --target gres/testui.c --dependency-file gres/testui.c.d
../test cases/vala/14 target glib version and gresources/gres/test-resources.xml: Failed to close file descriptor for child process (Operation not permitted).
[2/6] /usr/bin/glib-compile-resources '../test cases/vala/14 target glib version and gresources/gres/test-resources.xml' --sourcedir '../test cases/vala/14 target glib version and gresources/gres/.' --sourcedir '../test cases/vala/14 target glib version and gresources/gres' --internal --generate --target gres/testui.h
FAILED: gres/testui.h 
/usr/bin/glib-compile-resources '../test cases/vala/14 target glib version and gresources/gres/test-resources.xml' --sourcedir '../test cases/vala/14 target glib version and gresources/gres/.' --sourcedir '../test cases/vala/14 target glib version and gresources/gres' --internal --generate --target gres/testui.h
../test cases/vala/14 target glib version and gresources/gres/test-resources.xml: Failed to close file descriptor for child process (Operation not permitted).

Only the modification of install.sh triggers the issue. Any idea on this?
I don't know how to make the automated test otherwise.

@eli-schwartz
Copy link
Member

This happens in any PR that tries to modify the CI images, not just yours. It also causes the weekly cron image rebuilds to fail. This is not your problem, in other words.

The issue has been discovered elsewhere too, particularly https://gitlab.gnome.org/Infrastructure/GitLab/-/issues/545 which analyses it quite a bit. Ultimately I think we're at the mercy of Github's infrastructure, which I'm not sure we can do anything about and which has a timeframe I possess no knowledge of.

On other PRs which did CI updates on images other than the ones which currently fail to rebuild, I merged the CI image update separately, expected a bunch of jobs to fail, and just wanted to get the Ubuntu image to rebuild. See #10070

Unfortunately, that doesn't help if you actually want to update the Arch or OpenSUSE images.

@rtbo
Copy link
Contributor Author

rtbo commented Mar 7, 2022

I've changed to only modify the Ubuntu images, but it still leads to failure the Arch and Opensuse image builders.
I believe that "linux / Ubuntu Rolling" are failing due to being based on a previous image, is that correct?

@tristan957
Copy link
Contributor

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2022

This pull request introduces 1 alert when merging 6d12a27 into 7df6c6a - view on LGTM.com

new alerts:

  • 1 for Unused import

@rtbo
Copy link
Contributor Author

rtbo commented Mar 9, 2022

There is a bug in the Windows 32bits environment (vc2017x86ninja), but IMO not from my code:

  • At least for the 2 failing tests, Meson is configured to build for x86_64 host machine.

    • I don't know if it is on purpose to do this cross compilation, but I don't think so.
    • The trace shows that all machines (build, host and target) are x86_64. Is it a Meson bug?
  • My code looks for dependencies for the host machine.

    • For d/11 dub: it fails to find the dependency because only the x86 version of urld is installed
    • For d/14 dub with deps: it finds the dependencies because I install the right ones with run_command in the meson.build script.
  • d/14 dub with deps fail to link because the compiler generates 32bit code, despite host-machine (and dependencies) being x86_64. (The arch flags are based on compiler.arch, not host_machine.cpu_family()). This is also a bug (I guess).

For the time being I can skip the tests on Windows, but my code might break some situations that are working today (maybe shouldn't) on 32bits Windows. (arguablly not a large audience).

@lgtm-com
Copy link

lgtm-com bot commented Mar 10, 2022

This pull request introduces 1 alert when merging ab95e56 into 44104e8 - view on LGTM.com

new alerts:

  • 1 for Unused import

@rtbo rtbo force-pushed the fix_dub_deps branch 2 times, most recently from ab7eb15 to f3dd59a Compare March 11, 2022 08:09
@rtbo
Copy link
Contributor Author

rtbo commented Mar 11, 2022

I thought vc2017x86ninja was 32bit, but it is 64bit Windows with 32bit toolchain.
The current behavior of Meson is to have x86_64 host_machine and build_machine, but to generate 32bits binaries.
If this is intended and by design, my current code is correct and reflects this behavior.

Please let me know if anything must be changed.

rtbo added 2 commits March 12, 2022 10:39
 - fix the research of target built by DUB
 - explicitely state that DUB dynamic libraries and source libraries are not supported (yet) (mesonbuild#6581)
 - fix the build settings of recipes having sub-dependencies (mesonbuild#7560)
 - fix winlibs added from dub recipe
 - sanitization, comments, explanations...
@rtbo rtbo changed the title Fix dub deps Fix Dub dependencies Mar 12, 2022
@rtbo
Copy link
Contributor Author

rtbo commented Mar 22, 2022

Anything I can do to help the review?
On my side I think the code is ready for merge.

@dcbaker dcbaker self-assigned this Mar 22, 2022
@dcbaker
Copy link
Member

dcbaker commented Mar 22, 2022

I’ll look at this in a little bit, just been waiting for the merge window to open

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for persisting with this

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.

4 participants