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

Use the stable stdx-allocator DUB package #1983

Merged
merged 6 commits into from
Dec 11, 2017

Conversation

wilzbach
Copy link
Member

As discussed before, stdx-allocator is a frozen version of std.experimental.allocator to avoid the frequent breakages.

See also: dlang-community/discussions#27

@andralex
Copy link

Like! Thx @wilzbach

@s-ludwig
Copy link
Member

Thanks!

2.072 and 2.073 still fail with

source/stdx/allocator/building_blocks/affix_allocator.d(181,54): Error: function vibe.utils.hashmap.HashMap!(ulong, TimerInfo, DefaultHashMapTraits!ulong).HashMap.AW.resolveInternalPointer (void* p) is not callable using argument types (const(void*), void[])

And we need to make a Meson package for stdx-allocator, so that the Meson build continues to work.

@wilzbach
Copy link
Member Author

And we need to make a Meson package for stdx-allocator, so that the Meson build continues to work.
Okay I think I need a bit of help for the meson build.

CC @ximion

When I try to build the vibe-d package locally with meson, I get the following error:

The Meson build system
Version: 0.43.0
Source dir: /home/seb/dlang/vibe/vibe.d-repo
Build dir: /home/seb/dlang/vibe/vibe.d-repo/test
Build type: native build
Project name: Vibe.d
Native D compiler: ldc2 (llvm 1.5.0)
Build machine cpu family: x86_64
Build machine cpu: x86_64
Found pkg-config: /usr/bin/pkg-config (0.29.2)
Native dependency zlib found: YES 1.2.11
Native dependency libcrypto found: YES 1.1.0g
Native dependency libssl found: YES 1.1.0g
Native dependency libevent found: YES 2.1.8-stable
Cloning into 'diet'...
remote: Counting objects: 823, done.
remote: Total 823 (delta 0), reused 0 (delta 0), pack-reused 823
Receiving objects: 100% (823/823), 207.01 KiB | 566.00 KiB/s, done.
Resolving deltas: 100% (427/427), done.

Executing subproject diet.

Project name: Diet-NG
Native D compiler: ldc2 (llvm 1.5.0)
Also couldn't find a fallback subproject in lib/subprojects/diet for the dependency diet

Which is weird, because it detects that it needs the package and clones it correctly.
So I added more or less blindly a meson build spec to the stdx-allocator package and tried to copy the diet-ng dependency line, but I guess I have to do a bit more?

@ximion
Copy link
Contributor

ximion commented Nov 21, 2017

@wilzbach

When I try to build the vibe-d package locally with meson, I get the following error:

You did hit one of the dumbest Meson bugs: It Meson is unable to configure a subproject, it will throw a "Project not found" issue instead of telling you what the actual problem is.
Can you try configuring the cloned copy of Diet manually, and see if that works? (I bet there is some error which prevents the build from happening).

The stdx-allocator Meson build file looks good to me, all you need for Vibe is adding a wrap file like https://github.com/vibe-d/vibe.d/blob/master/lib/subprojects/diet.wrap and reference it like https://github.com/vibe-d/vibe.d/blob/master/meson.build#L58 - it also needs to be added to the dependency line of each target that uses it (like in https://github.com/vibe-d/vibe.d/blob/master/web/meson.build#L52 - I should probably have made a variable for this, so dependencies could be set globally).

@wilzbach wilzbach force-pushed the stdx-allocator branch 2 times, most recently from 43f9712 to 1333e32 Compare December 1, 2017 04:31
@wilzbach
Copy link
Member Author

wilzbach commented Dec 1, 2017

You did hit one of the dumbest Meson bugs:

Hmm, it turns out that my problem was related to using Meson 0.43. When I downgraded to 0.42 everything worked fine.

I should probably have made a variable for this, so dependencies could be set globally).

Once I realized that I manually have to update all meson build files and add the allocator_dep dependency variable to all build targets and update all include plus link_with variables, I started to switch to using dependencies and interestingly everything builds fine on my machine and now meson is able to automatically infer dependencies and their respective transitive dependencies.
This means that in a later PR, the dependency on e.g. ssl can be moved from vibe_utils to only the package which need it (here vibe_tls) and so on.
I know that changing all meson build files makes this PR quite large, but as I mentioned due to the current setup I have to touch them anyhow.

@ximion As you are the sole user of this meson setup here, please let me know if Meson dependencies like I have used them here would work for you. Thanks.

@ximion
Copy link
Contributor

ximion commented Dec 1, 2017

@wilzbach I found the cause for this issue by luck when reporting a - what I initially thought - different request to Meson. See mesonbuild/meson#2719 - depending on what the outcome of that discussion there is we might need to change the directory of the .wrap files to some toplevel dir if that is okay with @s-ludwig.

On the Meson file changes: They look good, and this is the way this should have been done initially. Thank you very much for doing all that work! :-)
I wonder why it wasn't done like that from the start, and I guess that was because the requires property of the pkg-config generate function in Meson didn't exist ages ago (and to the day isn't documented, which should be fixed).
That being said, the requires property should likely be added to make linking to individual Vibe modules more fun for other projects.

version: project_version,
soversion: project_soversion
)
pkgc.generate(name: 'vibe-core',
libraries: [vibe_core_lib] + core_link_with,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should instead gain a requires: ['vibe-utils', 'vibe-data'] line to ensure that if someone links a project with vibe-core, those two get linked to as well (since pretty much all of vibe-core depends on utils and data in its public routines).
This applies to all of these instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. Done for all meson files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot! I haven't tested this yet, but the Meson files themselves look fine to me, even much better than before :-)

http/meson.build Outdated
vibe_inet_dep,
vibe_tls_dep,
vibe_crypto_dep,
diet_dep],
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting got weird, in a couple of other occasions below as well.

@wilzbach
Copy link
Member Author

wilzbach commented Dec 1, 2017

I found the cause for this issue by luck when reporting a - what I initially thought - different request to Meson.

Thanks a lot for looking into this!

On the Meson file changes: They look good, and this is the way this should have been done initially.

Thanks! If we already talk about Meson there are two things that I find rather irritating:

  • The test target feels rather redundant. I would have expected that one can extend/inherit or copy the lib target
  • We are declaring the dependencies 3x (lib target, dependency, test target). Is there a better way of doing this? I guess declaring a dependencies array initially would already be a small win.

@ximion
Copy link
Contributor

ximion commented Dec 1, 2017

The test target feels rather redundant. I would have expected that one can extend/inherit or copy the lib target

This in a way is the "fault" of D's feature of embedding unittests. In any other language, especially C/C++ you will compile dedicated binaries which contain the tests, which can then be installed to some places and run as self-tests or just be run upon a make check. Meson just accounts for all possibilities here, making this as generic as possible for all languages (including Java and Rust).
Making a deep copy of a target while changing its name and later adjusting the target parameters would be really nice here, but I doubt that a patch for that would be accepted (the Meson authors want to keep the amount of scripting at an absolute minimum, and adding this would require quite some changes as well).
On a sidenote, none of the Meson developers knew that D or any language had this feature to compile the same source twice to receive the application or the unittests depending on the compile flags used.

We are declaring the dependencies 3x (lib target, dependency, test target). Is there a better way of doing this? I guess declaring a dependencies array initially would already be a small win.

Yes, that's what I would suggest as well. The dependencies line for all of these could be put into a variable and just be reused. AFAIK there is no other way to do this at the moment.

Btw, a small summary on how to use D with Meson exists for a while now: http://mesonbuild.com/D.html

@wilzbach
Copy link
Member Author

wilzbach commented Dec 2, 2017

Btw the vibe-core failures will be fixed by vibe-d/vibe-core#43, otherwise we are more or less green.
As mentioned at vibe-d/vibe-core#43 (comment), stdx-allocator only works with >= 2.072 (>= LDC 1.2) as there have been many safety-related changes. Please let me know if keeping support for 2.071 is an absolute must.

@ximion
Copy link
Contributor

ximion commented Dec 5, 2017

@s-ludwig Is vibe-core ready, btw? In other words, should it get a Meson definition and should the libevent linkage dropped by default soon? (in Debian I am still building it with libevent, because I thought that was the recommended option back then)

@wilzbach
Copy link
Member Author

wilzbach commented Dec 6, 2017

Okay so now it looks a lot better:

image

image

Only DMD 2.071.1 and LDC 1.1.0 failing - as discussed at vibe-core, I removed those and hopefully now this PR finally gets green.

@s-ludwig Is vibe-core ready, btw? In other words, should it get a Meson definition and should the libevent linkage dropped by default soon? (in Debian I am still building it with libevent, because I thought that was the recommended option back then)

I use vibe-core for my projects actively and vibe-core had its 1.0.0 release this summer. For DUB the old vibe.core package with libevent is still the default, but I assume @s-ludwig is going to swap the defaults with the next Vibe.d release soon (and possibly even remove the old vibe.core package?)

@wilzbach
Copy link
Member Author

wilzbach commented Dec 6, 2017

Alrighty - finally everything is green :)

@s-ludwig s-ludwig merged commit ecc1a42 into vibe-d:master Dec 11, 2017
@wilzbach wilzbach deleted the stdx-allocator branch December 12, 2017 00:37
@wilzbach
Copy link
Member Author

Thanks a lot for moving forward with this!
Could I bother you to release a 0.8.3-alpha.1 tag, s.t. DUB built with Vibe.d can pick this change up as well and doesn't fail in the Project Tester either?

@s-ludwig
Copy link
Member

Yep, will do after the compatibility code PR is merged. And thanks again for your help with the allocator stuff!

@s-ludwig
Copy link
Member

Available now on code.dlang.org.

@MartinNowak
Copy link
Contributor

It's a good hint that developing in std.experimental is a worse choice than developing sth. as a dub package.

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.

5 participants