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.

Sign up for GitHub

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

Jump to bottom

[zstd] Removing de-duplication of cmake flags #13454

Merged
merged 5 commits into from
Oct 27, 2020
Merged

[zstd] Removing de-duplication of cmake flags #13454

merged 5 commits into from
Oct 27, 2020

Conversation

Nemirtingas
Copy link
Contributor

Describe the pull request

  • What does your PR fix?
    Fixes cross-compilation of zstd linux to windows.

NancyLi1013
NancyLi1013 reviewed Sep 11, 2020
View reviewed changes
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

Could you please also update CONTROL file?

@NancyLi1013 NancyLi1013 added category:port-bug The issue is with a library, which is something the port should already support requires:author-response labels Sep 11, 2020
@NancyLi1013 NancyLi1013 changed the title Fix zstd until https://github.com/facebook/zstd/pull/2205 is merged [zstd] Removing de-duplication of cmake flags Sep 11, 2020
@Nemirtingas
Copy link
Contributor Author

If I understand correctly the documentation, I just need to increase the port-version right ?
I just did it, can you check please ?

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Sep 14, 2020
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your PR.

@Nemirtingas Nemirtingas deleted the zstd_crosscompile_fix branch September 23, 2020 09:22
@NancyLi1013
Copy link
Contributor

Hi @Nemirtingas
Why do you close this PR? It looks good on my side.

@Nemirtingas Nemirtingas restored the zstd_crosscompile_fix branch September 24, 2020 07:12
@Nemirtingas Nemirtingas reopened this Sep 24, 2020
@Nemirtingas
Copy link
Contributor Author

Hi, sorry, I deleted the branch locally and it deleted the remote branch. It was a mistake.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

NancyLi1013 commented Sep 25, 2020 •
edited
Loading

Is it possible to merge master to this PR to retrigger CI?

Edit: Please do this after PR #13722 merged.

@NancyLi1013 NancyLi1013 removed the info:reviewed Pull Request changes follow basic guidelines label Sep 25, 2020
@Nemirtingas
Copy link
Contributor Author

Is this ok to merge ?

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@NancyLi1013
Copy link
Contributor

LGTM now.
I think it can be merged once CI checks have passed.

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Oct 14, 2020
@dan-shaw
Copy link
Contributor

Can we submit this patch to upstream instead?

@dan-shaw dan-shaw added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Oct 14, 2020
@NancyLi1013
Copy link
Contributor

I checked the source code in upstream.
This change has been merged to upstream, but not in current version.
facebook/zstd@0fa9406#diff-c2115e00ec0f36ee352263ce07b253b1ffb80fc32cf0be0cf04145bdabea6221

We might consider to remove this patch in next release. What do you think? @dan-shaw

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Oct 27, 2020
@BillyONeal BillyONeal merged commit c7db4c3 into microsoft:master Oct 27, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution

@Nemirtingas Nemirtingas deleted the zstd_crosscompile_fix branch November 22, 2020 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Nemirtingas @NancyLi1013 @dan-shaw @BillyONeal @GameDumper