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

Allow custom dflags to be defined for dependencies #2040

Merged
merged 3 commits into from
Dec 7, 2020

Conversation

omerfirmak
Copy link
Contributor

For example;

{
	"authors": [
		"omer"
	],
	"copyright": "Copyright © 2020, omer",
	"description": "A simple vibe.d server application.",
	"license": "proprietary",
	"name": "dubtest",

	"dependencies": {
		"vibe-d": { "version" : "~>0.9.2", "dflags" : ["-preview=in"] }
	}
}

-preview=in will be applied to vibe-d and all of its dependencies. Any BuildSettings field can be used, parsing support is present. But, for now only, dflags is taken into account when compiling.

@dlang-bot
Copy link
Collaborator

Thanks for your pull request and interest in making D better, @omerfirmak! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

@omerfirmak
Copy link
Contributor Author

Would this need a changelog entry?

@Geod24
Copy link
Member

Geod24 commented Nov 25, 2020

Definitely. CC @s-ludwig , how do you feel about this ? The issue is that currently, we cannot propagate flags from the parent packages to the child packages. Things like -fPIC or -preview switches. I mentioned it in the DConf talk.

@Geod24
Copy link
Member

Geod24 commented Nov 25, 2020

And a test :)

"name": "depen-build-settings",

"dependencies": {
"depend1": { "version" : "*", "dflags" : ["-version=depen_build_settings"] }
Copy link
Member

Choose a reason for hiding this comment

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

Using -version is a pretty bad example, because DUB handles it, and will give you a warning on this.
Use something like -fPIC instead, something that needs to be propagated and won't go away.

Copy link
Contributor Author

@omerfirmak omerfirmak Nov 25, 2020

Choose a reason for hiding this comment

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

I dont see any warning while building, at which point should I receive it? Both depen1 and depend2 need -version=depen_build_settings to successfully compile. If I remove or change it to something else, compilation fails as expected.

depen1 depends on depen2, so test also checks that the flags are applied transitively to all of the dependency chain.

Copy link
Member

Choose a reason for hiding this comment

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

{
    "name": "mytest",
    "dflags": [ "-version=Hello" ]
}

Leads to:

$ dub build                                                                                                                                                          :(

## Warning for package mytest ##

The following compiler flags have been specified in the package description
file. They are handled by DUB and direct use in packages is discouraged.
Alternatively, you can set the DFLAGS environment variable to pass custom flags
to the compiler, or use one of the suggestions below:

-version=Hello: Use "versions" to specify version constants in a compiler independent way

Performing "debug" build using dmd for x86_64.
mytest ~master: building configuration "application"...
Linking...

@Geod24
Copy link
Member

Geod24 commented Nov 25, 2020

Also the test needs to make sure that the dflags is transitive otherwise it's not of much use to the user.

@Geod24
Copy link
Member

Geod24 commented Nov 25, 2020

Would solve #279

@andre2007
Copy link
Contributor

andre2007 commented Nov 25, 2020

Changing the dflags of a dependency should also trigger a new compilation of the dependency. Maybe a check for this could also be added to the test.

@omerfirmak omerfirmak force-pushed the agora-1327 branch 2 times, most recently from 0156d69 to af2b316 Compare November 26, 2020 12:56
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

LGTM, any opinion @PetarKirov @s-ludwig @wilzbach ?

@omerfirmak omerfirmak force-pushed the agora-1327 branch 2 times, most recently from f5b6e8b to e61a1d5 Compare November 27, 2020 09:03
Comment on lines 4 to 14
`
{
"description": "A simple vibe.d server application.",
"license": "proprietary",
"name": "dubtest",

"dependencies": {
"vibe-d": { "version" : "~>0.9.2", "dflags" : ["-preview=in"] }
}
}
`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`
{
"description": "A simple vibe.d server application.",
"license": "proprietary",
"name": "dubtest",
"dependencies": {
"vibe-d": { "version" : "~>0.9.2", "dflags" : ["-preview=in"] }
}
}
`
---
{
"description": "A simple vibe.d server application.",
"license": "proprietary",
"name": "dubtest",
"dependencies": {
"vibe-d": { "version" : "~>0.9.2", "dflags" : ["-preview=in"] }
}
}
---

And there seems to be some tabs in there

For now, only "dflags" is used
@Geod24
Copy link
Member

Geod24 commented Dec 7, 2020

Fixed the changelog (and slightly improved it), merging since no opposition was voiced.

@Geod24 Geod24 merged commit cd9b30e into dlang:master Dec 7, 2020
@omerfirmak omerfirmak deleted the agora-1327 branch December 7, 2020 19:30
@Geod24
Copy link
Member

Geod24 commented Jan 11, 2021

@omerfirmak : This is missing the implementation for SDL.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants