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

[stable] Deduplicate dflags and lflags #2016

Merged
merged 2 commits into from
Sep 28, 2020
Merged

[stable] Deduplicate dflags and lflags #2016

merged 2 commits into from
Sep 28, 2020

Conversation

Geod24
Copy link
Member

@Geod24 Geod24 commented Sep 28, 2020

Before:
Screen Shot 2020-09-28 at 12 03 40

After:
Screen Shot 2020-09-28 at 12 04 49

There are still some improvements needed (notice the duplicated -L-framework), but the diff / benefit ratio is already great.
Packed in another tiny cleanup to avoid endless CI retriggers.
Targeting stable because I'm working on propagating dflags downwards and obviously this will come in handy.

getPackageSuppliers already handles SkipPackageSuppliers.none correctly,
no need for an extra check.
For some reason, dflags and lflags were not using the 'add' convenience function,
which allows to deduplicate flags.
As a result some flags, especially link flags which propagate upwards,
ended up taking half of the command line length.
@dlang-bot
Copy link
Collaborator

Thanks for your pull request, @Geod24!

@Geod24
Copy link
Member Author

Geod24 commented Sep 28, 2020

I believe that was also a pet peeves of yours @UplinkCoder

@Geod24 Geod24 merged commit 10bea6f into dlang:stable Sep 28, 2020
@Geod24 Geod24 deleted the dedup branch September 28, 2020 04:44
@s-ludwig
Copy link
Member

But -L-framework needs to be duplicated for every framework - won't this result in a wrong outcome?

lflags "-L-framework" "-LCoreServices" "-L-framework" "-LCoreFoundation"

should get -L-framework -LCoreServices -LCoreFoundation with these changes, meaning that "CoreFoundation" is passed to the linker as a normal file argument instead of as a framework name.

As far as I can see, this operates on the wrong level and needs to be reverted.

@Geod24
Copy link
Member Author

Geod24 commented Sep 28, 2020

Look at the second screenshot: It is still duplicated.
However, I didn't knew this bit about -L-framework, and I have no idea why this is still duplicated. I'll take a look.

@s-ludwig
Copy link
Member

Yes, I've seen the screenshot, but that does not look like the outcome of the algorithm, but of some coincident external effect.

s-ludwig added a commit that referenced this pull request Oct 1, 2020
PR #2016 introducted a de-duplication strategy that is too eager and corrupts argument chains, such as "-framework CoreServices -framework CoreFoundation" where the "-framework" argument needs to be duplicated for each framework.

This reverts commit 10bea6f.
@s-ludwig
Copy link
Member

s-ludwig commented Oct 1, 2020

By the way, I know how annoying it is to miss a release, but in this case it was quite lucky that it came too late for the latest release. It really shouldn't have gone to the stable branch ;-)

Geod24 pushed a commit that referenced this pull request Oct 1, 2020
PR #2016 introducted a de-duplication strategy that is too eager and corrupts argument chains, such as "-framework CoreServices -framework CoreFoundation" where the "-framework" argument needs to be duplicated for each framework.

This reverts commit 10bea6f.
@MartinNowak MartinNowak mentioned this pull request Oct 10, 2020
PetarKirov pushed a commit that referenced this pull request Oct 10, 2020
PR #2016 introducted a de-duplication strategy that is too eager and corrupts argument chains, such as "-framework CoreServices -framework CoreFoundation" where the "-framework" argument needs to be duplicated for each framework.

This reverts commit 10bea6f.
@MartinNowak MartinNowak mentioned this pull request Oct 18, 2020
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