open-scene-graph: fix location of collada-dom includes #46380
Conversation
| @@ -62,7 +62,9 @@ def install | |||
| end | |||
|
|
|||
| if build.with? "collada-dom" | |||
| args << "-DCOLLADA_INCLUDE_DIR=#{Formula["collada-dom"].opt_include}/collada-dom" | |||
| # https://github.com/Homebrew/homebrew/issues/43536 | |||
| collada_include_dir = `ls -d #{Formula["collada-dom"].opt_include}/collada-dom*` | |||
There was a problem hiding this comment.
Would be good to use Ruby logic here instead e.g. the Dir class.
There was a problem hiding this comment.
Updated it to use Ruby's Dir.
|
Closing this because it still fails with other issues. Upstream build script can't locate the |
|
Reopening; got it fixed by explicitly setting COLLADA_DOM_DIR based on COLLADA_INCLUDE_DIR. |
|
👍 once 🍏 |
| # https://github.com/Homebrew/homebrew/issues/43536 | ||
| collada_include_candidates = Dir.glob("#{Formula["collada-dom"].opt_include}/collada-dom*") | ||
| if collada_include_candidates.empty? | ||
| odie "Could not locate collada-dom include directory" |
There was a problem hiding this comment.
From the discussion in one of my PRs, I learned it's better to use raise instead of odie.
|
Test is failing on 10.11 due to #46372. |
| if collada_include_candidates.empty? | ||
| odie "Could not locate collada-dom include directory" | ||
| end | ||
| collada_include_dir = collada_include_candidates[0] |
There was a problem hiding this comment.
My impression was that using .first in these instances is more common than array indexing with [0]. You could also move the preceding check after this statement, making the code a bit more compact:
collada_include_dir = Dir.glob("#{Formula["collada-dom"].opt_include}/collada-dom*").first
raise "Could not locate collada-dom include directory" unless collada_include_dir(But maybe the code is too wide to warrant using unless as a statement modifier.)
There was a problem hiding this comment.
That looks more readable. I switched it to use this form, and raise instead of odie.
|
Closing this to avoid spurious test jobs. I'll resubmit this and the other |
Fixes #43536