Skip to content

Conversation

@myqi
Copy link

@myqi myqi commented Sep 28, 2018

Signed-off-by: Mingyuan Qi [email protected]

Copy link

@saulwold saulwold left a comment

Choose a reason for hiding this comment

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

The 3 patches that you have added need to be upstreamed to sm-db as part of the multi-os effort, you should also include more details about "why" those patches are needed, for example, just having a patch subject of "reorder LDLIBS" does not explain "why". Please include what compile error you are solving as the "why". Please keep this in mind when submitting patches here.

@myqi
Copy link
Author

myqi commented Sep 29, 2018

@saulwold for 0002 and 0003 patch, I agree they need to be upstreamed since they're real issue. For 0001, there're lots of warnings that lower version gcc in centos didn't care. It's another task we can address I think, but out of clear scope. As for commit message, it's predefined and auto generated. I will add more messages to each kind of patch.

Copy link

@saulwold saulwold left a comment

Choose a reason for hiding this comment

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

Not sure I completely agree about 0001, the correct fix is to address the warning and upstream them, as part of Multi-OS (which is imporant), I think it's very much part of what we need to be doing. Just because CentOS does not care does not mean they should not be fixed.

@myqi
Copy link
Author

myqi commented Oct 17, 2018

@saulwold I think you misunderstood what I mean, I agree with you about 0001 that this patch itself is meaningless, it needs to be fixed with the real warning issue and upstream instead of just turn off warning. That's what I mean "another task", it's related to multi-os, not clear only. Anyway, these issues have to be upstreamed during clear enabling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants