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

fix #139 commit protobuf lite and a model for test #140

Merged
merged 2 commits into from
Apr 25, 2018
Merged

fix #139 commit protobuf lite and a model for test #140

merged 2 commits into from
Apr 25, 2018

Conversation

RayLiu2015
Copy link
Collaborator

fix #139 commit protobuf lite and a model for test

@Xreki
Copy link
Contributor

Xreki commented Apr 25, 2018

在paddle-mobile repo中,我们尽量只维护我们自己的代码+必要的脚本。所以有以下几个建议:

  • protobuf-lite库不建议以这种方式加进去,这会引入很多不属于这个repo的源码。另外,如果我们后面要更新或替换protobuf-lite库,也会引入很多不属于这个repo的改动。
  • 不要在这个repo中加入fluid的模型,至于怎么进行测试,我们需要一个单测方案
  • 源码中只维护.proto源文件,.pb.h.pb.cc是由protoc编译生成的,不应该维护在源码中,否则容易造成源码不一致。另外,mobile的framework.proto设计似乎是与Paddle Fluid有所不同,我建议先只增加mobile需要的部分。
  • 不要添加没有实现内容的源文件。建议等具体实现的时候再添加。
  • 文件头需要添加copyright。

@panyx0718
Copy link
Contributor

源代码中应该不包含编译后生成的文件的。

@panyx0718
Copy link
Contributor

第三方代码(protobuf)应该是在编译时clone下来,或者其他方式动态获取?而不是存在paddle-mobile的代码库里?

@RayLiu2015
Copy link
Collaborator Author

关于 protobuf 这个问题, protobuf 是需要编译器clone 下来的吗? 然后根据clone 下来的protoc 生成 framework.pb.h ?
但 paddle-mobile遇到的情况是, 我们只需要一个 protobuf-lite, 而不需要其他的部分, 我们没有必要把整个protobuf clone 下来, framework.pb.h 提前生成好也可以和当前protobuf-lite 版本绑定

@allonli
Copy link
Collaborator

allonli commented Apr 25, 2018

这个二进制文件只是前期使用,为了稳定运行而用。后面会删除重新实现。 @Xreki @panyx0718

@panyx0718
Copy link
Contributor

好的。将来删掉重新实现可能要注意清理git的历史,可能会导致git代码库比较大。

@Xreki
Copy link
Contributor

Xreki commented Apr 25, 2018

我个人还是觉得直接将protobuf-lite的头文件和库加入到repo里面不妥,git历史不好清理。对于protobuf-lite,我有3种方案,大家可以看下如何:

  1. 用户在cmake时手动指定PROTOBUF_ROOT,这样要求用户本地有编译好的protobuf库
  2. 将protobuf库打包上传到某个地方,由cmake下载
  3. 将protobuf库上传到私人的github repo,以sub-module的方式引入

这3种方式,都不会对paddle-mobile造成巨大的改动历史。

Copy link
Collaborator

@allonli allonli left a comment

Choose a reason for hiding this comment

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

尽量使用头文件方式,临时加入,后续删除。后面可以对his整个目录删除。操作上要注意一下。

@allonli allonli added the todo label Apr 25, 2018
@allonli allonli merged commit 8edfa0d into PaddlePaddle:develop Apr 25, 2018
@allonli
Copy link
Collaborator

allonli commented Apr 25, 2018

merged + todo label

@emailweixu
Copy link

Should use ExternalProject or git submodule for protobuf-lite. It make no sense to directly add it into our repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add protobuf-lite
5 participants