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

Qualify NativePath members #2407

Closed
wants to merge 2 commits into from

Conversation

nordlow
Copy link
Contributor

@nordlow nordlow commented Aug 18, 2022

Trivial. Ready for merge.

@nordlow nordlow changed the title Qualifyy NativePath members Qualify NativePath members Aug 18, 2022
@Geod24
Copy link
Member

Geod24 commented Aug 18, 2022

Doesn't compile because Vibe.d has different qualifiers.

@nordlow
Copy link
Contributor Author

nordlow commented Aug 18, 2022

Doesn't compile because Vibe.d has different qualifiers.

Oops, thanks. Forgot to compile with unittests.

Fixed via https://github.com/dlang/dub/compare/e230fd0234ac42c60af9eeefefa6611c72e65072..fcbed3642763e9db8c2189e8f2e2eb9ec1400ad4.

dub test pass locally now. Ok to merge.

@nordlow
Copy link
Contributor Author

nordlow commented Aug 18, 2022

Ehh, I'm clueless about the CI errors. How can a simple adding of qualifiers break CI in any way when dub test passes locally?

@Geod24
Copy link
Member

Geod24 commented Aug 18, 2022

How do you compile?
Test what the CI is doing. Building optionally with vibe-core is probably what breaks it.

@nordlow
Copy link
Contributor Author

nordlow commented Aug 18, 2022

Building optionally with vibe-core is probably what breaks it.

How do I do that? I find no reference to vibe-core in top dub.sdl.

Do you mean any of the dub.sdl-configurations

configuration "library-nonet" {
	dependency "vibe-d:http" version=">=0.9.0 <0.10.0" optional=true
	targetType "library"
	excludedSourceFiles "source/app.d"
}

configuration "dynamic-library-nonet" {
	dependency "vibe-d:http" version=">=0.9.0 <0.10.0" optional=true
	targetType "dynamicLibrary"
	excludedSourceFiles "source/app.d"
}

?

@nordlow
Copy link
Contributor Author

nordlow commented Aug 19, 2022

How do you compile? Test what the CI is doing. Building optionally with vibe-core is probably what breaks it.

Yes, but the problem is that the travis.sh script fails locally in on master and I find it really to hard to understand its output. We really need to do something about its output so it becomes obvious what are errors and not.

@nordlow
Copy link
Contributor Author

nordlow commented Aug 19, 2022

How do you compile?

I do dub test locally.

Test what the CI is doing. Building optionally with vibe-core is probably what breaks it.

Grepping for vibe-core gives no hits. I don't understand what you mean.

@Geod24 Geod24 force-pushed the qualify-NativePath-members branch 5 times, most recently from 5160c06 to e81aaf3 Compare October 27, 2022 14:22
@Geod24
Copy link
Member

Geod24 commented Oct 27, 2022

Modified and rebased but surprisingly fails in UT...

Geod24 and others added 2 commits October 27, 2022 18:02
We want to improve our path abstraction,
which we can't do if we rely on Vibe's.
One immediate improvement will be to add proper
attributes to methods.
@nordlow
Copy link
Contributor Author

nordlow commented Oct 27, 2022

Any clues why some platforms are passing and some are not?

@Geod24
Copy link
Member

Geod24 commented Oct 27, 2022

Older compilers don't run test, they just compile

{
m_nodes = nodes;
m_absolute = absolute;
}

/// Constructs a relative path with one path entry.
this(PathEntry entry){
this(PathEntry entry) { pure nothrow
Copy link
Member

Choose a reason for hiding this comment

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

broken attributes

source/dub/internal/vibecompat/inet/path.d Show resolved Hide resolved
@Geod24
Copy link
Member

Geod24 commented Jun 18, 2024

I recommend to go with something like #2821 instead.
Closing as stalled.

@Geod24 Geod24 closed this Jun 18, 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.

3 participants