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

Improved wording of the cmake warning messages for ASSIMP #553

Merged
merged 2 commits into from
Dec 5, 2015

Conversation

mxgrey
Copy link
Member

@mxgrey mxgrey commented Nov 10, 2015

I felt that the wording of the cmake warning messages for ASSIMP might be difficult to understand, so I attempted to improve their clarity a bit.

@psigen
Copy link
Collaborator

psigen commented Nov 10, 2015

👍 These are a lot clearer.

Do we know which version of Assimp this issue was fixed in? It may be helpful to specify "Assimp 3.0.0+" or something, since I wouldn't know what version of Assimp to try if this issue came up.

@jslee02 jslee02 added this to the DART 6.0.0 milestone Nov 10, 2015
@jslee02
Copy link
Member

jslee02 commented Nov 10, 2015

Looks better now 👍

There are two classes that Assimp 3.0~dfsg-3 (package in vivid) is missing: aiMaterial and aiScene. The symbols of aiMaterial were included in Assimp 3.0~dfsg-4 (package in wily) but not of aiScene. So any released package of Assimp has not completely fixed the issue yet.

@mxgrey
Copy link
Member Author

mxgrey commented Nov 10, 2015

Then maybe we should add "once one becomes available" to the warning message.

We wouldn't want to send users on a futile search for a library version that doesn't exist.

@jslee02
Copy link
Member

jslee02 commented Nov 10, 2015

Good idea.

I haven't tested yet for all the platforms again, but it seems only the versions of Assimp in the ubuntu package system have this issue. So I would like to update the recommendation sentence to "using a version of ASSIMP package that does not have this issue once one becomes available". Also, we could encourage people to build and install ASSIMP from the source.

@jslee02
Copy link
Member

jslee02 commented Nov 19, 2015

This branch needs to merge master into this branch in order to be merged into master back. No conflicts were sufficient condition to be merged into master when master was not a protected branch. However, now it seems to require to check if the changes of master, that was made after this branch was branched out, pass the status checks with this branch.

Regarding the warning message, just adding "once one becomes available" is also fine to me.

Edit: The requirement seems to be too restrictive so it's disabled. Now protected branches are just prevented from being deleted and force pushed.

@jslee02
Copy link
Member

jslee02 commented Dec 3, 2015

Either of @mxgrey 's suggestion and just current message looks good to me. I think we could merge this pull unless there is more idea.

@mxgrey
Copy link
Member Author

mxgrey commented Dec 3, 2015

I'll tweak the message tonight, and then I think it'll be ready to merge.

@jslee02
Copy link
Member

jslee02 commented Dec 3, 2015

Thanks, @mxgrey. Don't need to rush. Just wondered your think on this. Please tweak the message when you have a chance.

@mxgrey
Copy link
Member Author

mxgrey commented Dec 4, 2015

The comment has been updated. I believe this is ready to merge.

@jslee02
Copy link
Member

jslee02 commented Dec 5, 2015

👍

jslee02 added a commit that referenced this pull request Dec 5, 2015
Improved wording of the cmake warning messages for ASSIMP
@jslee02 jslee02 merged commit 4efda83 into master Dec 5, 2015
@mxgrey mxgrey deleted the grey/improve_cmake_warnings branch February 1, 2016 04:15
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.

3 participants