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

Added patch to USD for DTS bug #25

Conversation

aloysbaillet
Copy link
Contributor

See comments in Autodesk/maya-usd#198
This supersedes #24 as patch was not quite working and Eoin didn't use DCO...

Signed-off-by: Aloys Baillet <[email protected]>
Signed-off-by: Aloys Baillet <[email protected]>
jfpanisset
jfpanisset previously approved these changes Feb 12, 2020
Copy link
Contributor

@jfpanisset jfpanisset left a comment

Choose a reason for hiding this comment

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

My bash-fu is minimal, could the heredoc expression:

cat <<EOF | patch -p1

be shortened to:

patch -p1 <<EOF

or is there a reason for pushing the text of the patch through cat first?

Also for the change from make -j64 to make -j12, this article has some suggestions on using the output of nproc to set that variable based on the number of cores, which might be more adaptable to a build environment with a different number of available cores?

https://unix.stackexchange.com/questions/208568/how-to-determine-the-maximum-number-to-pass-to-make-j-option

@aloysbaillet
Copy link
Contributor Author

Thanks JF! Sorry took me a while to come back to this one... I think I used to use dynamic CPU number but got burnt by this on circle-ci as the machines had lots of CPUs and not enough memory so using them all would just crash the compiler... That said we seem good now on Azure :-)
Unfortunately new commits mean you'll have to re-approve the PR...

jfpanisset
jfpanisset previously approved these changes Feb 17, 2020
Copy link
Contributor

@jfpanisset jfpanisset left a comment

Choose a reason for hiding this comment

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

Hopefully CPU autoscaling will work out...

Signed-off-by: Aloys Baillet <[email protected]>
@aloysbaillet
Copy link
Contributor Author

Sorry I just realised I forgot to increase the version number for the docker image :-( sorry JF you'll need to re-approve yet again!

Copy link
Contributor

@jfpanisset jfpanisset left a comment

Choose a reason for hiding this comment

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

Looks good, approved.

@aloysbaillet aloysbaillet merged commit 3b1aec4 into AcademySoftwareFoundation:master Feb 19, 2020
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