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

[flatbuffers] upgrade the flatbuffers to v2.0.0 #18897

Merged
merged 21 commits into from Jul 22, 2021

Conversation

jixingcn
Copy link
Contributor

Describe the pull request

  • What does your PR fix?

    Upgrade the flatbuffers to v2.0.0

  • Which triplets are supported/not supported? Have you updated the [CI baseline]

    Yes

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

@JonLiu1993 JonLiu1993 self-assigned this Jul 12, 2021
@JonLiu1993 JonLiu1993 added the category:port-update The issue is with a library, which is requesting update new revision label Jul 12, 2021
ports/flatbuffers/CONTROL Outdated Show resolved Hide resolved
ports/mnn/vcpkg.json Outdated Show resolved Hide resolved
@JonLiu1993
Copy link
Member

@jixingcn ,Thanks for your pr.
have you tested the feature locally on x64-linux, x64-windows, and x64-windows-static?

@jixingcn
Copy link
Contributor Author

jixingcn commented Jul 12, 2021

@jixingcn ,Thanks for your pr.
have you tested the feature locally on x64-linux, x64-windows, and x64-windows-static?

My application use flatbuffers by x64-windows and x64-windiws-static. I think all features are tested or verified.

jixingcn and others added 2 commits July 12, 2021 15:56
Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>
Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>
@mathisloge
Copy link
Contributor

@JonLiu1993 @jixingcn it does seem a bit suspicious to add a 19000 lines patch file.
What is the context of this patch. Shouldn't it be a upstream change?

@JonLiu1993
Copy link
Member

@JonLiu1993 @jixingcn it does seem a bit suspicious to add a 19000 lines patch file.
What is the context of this patch. Shouldn't it be a upstream change?

Sorry, I did not find a patch file with 19000 lines. Can you point it out for me? Thank you

@JonLiu1993
Copy link
Member

@jixingcn,Please run the command "./vcpkg x-add-version flatbuffers --overwrite-version" and commit the changes again.

@jixingcn
Copy link
Contributor Author

@JonLiu1993 @jixingcn it does seem a bit suspicious to add a 19000 lines patch file.
What is the context of this patch. Shouldn't it be a upstream change?

Sorry, I did not find a patch file with 19000 lines. Can you point it out for me? Thank you

You can find in 96e57b8.

Commit this patch because the pipeline's report. The 19000+ lines were generated by MNN's schema generate script and the latest flatc that was compiled by vcpkg.

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 13, 2021
@jixingcn jixingcn requested a review from JonLiu1993 July 13, 2021 02:02
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

We cannot commit a 19k line patch like this, especially since it will need to change every time flatbuffers updates -- instead, mnn's port should run flatc at build time to generate any needed files.

You can easily get a runnable flatc.exe by depending on flatbuffers as a host dependency:

{
  "dependencies": [
    { "host": true, "name": "flatbuffers" }
  ]
}

Then, in the portfile, you can use ${CURRENT_HOST_INSTALLED_DIR}/tools/... to access the host dependency.

@JonLiu1993 JonLiu1993 added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jul 14, 2021
@jixingcn
Copy link
Contributor Author

We cannot commit a 19k line patch like this, especially since it will need to change every time flatbuffers updates -- instead, mnn's port should run flatc at build time to generate any needed files.

You can easily get a runnable flatc.exe by depending on flatbuffers as a host dependency:

{
  "dependencies": [
    { "host": true, "name": "flatbuffers" }
  ]
}

Then, in the portfile, you can use ${CURRENT_HOST_INSTALLED_DIR}/tools/... to access the host dependency.

OK, thanks. I will fix that.

@JonLiu1993
Copy link
Member

@jixingcn
Copy link
Contributor Author

versions/m-/mnn.json Outdated Show resolved Hide resolved
ports/mnn/vcpkg.json Outdated Show resolved Hide resolved
jixingcn and others added 3 commits July 15, 2021 14:39
Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>
Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>
@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jul 16, 2021
@JonLiu1993
Copy link
Member

@jixingcn ,Thanks for your contribution.

@jixingcn
Copy link
Contributor Author

@jixingcn ,Thanks for your contribution.

Thanks for your patient. 😃

@vicroms vicroms merged commit eea00aa into microsoft:master Jul 22, 2021
@jixingcn jixingcn deleted the upgrade-flatbuffers branch July 23, 2021 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants