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

CI Fix #3116

Closed
wants to merge 12 commits into from
Closed

CI Fix #3116

wants to merge 12 commits into from

Conversation

shubham1206agra
Copy link
Contributor

Updated macOS version in CMake file.
Removed warning in the Padding layer

Removed warning in padding layer
@zoq
Copy link
Member

zoq commented Dec 30, 2021

Thanks for looking into this.

@shubham1206agra
Copy link
Contributor Author

There is some issue with macOS 11 for R.
Downgrading to 10.15 works for now.

Copy link
Member

@shrit shrit left a comment

Choose a reason for hiding this comment

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

Is there any reason to downgrade like this?

CMakeLists.txt Outdated
Comment on lines 182 to 187
# extract V from '11.V.x'.
exec_program(/usr/bin/sw_vers ARGS
-productVersion OUTPUT_VARIABLE MACOSX_VERSION_RAW)
string(REGEX REPLACE
"10\\.([0-9]+).*" "\\1"
"11\\.([0-9]+).*" "\\1"
MACOSX_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this modification will work as you change the version to 10 now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is working as for now.
I was trying to fix the following.
ld: warning: dylib (/Users/runner/work/1/s/build/lib/libmlpack.dylib) was built for newer macOS version (11.6) than being linked (10.14)
ld: warning: dylib (/usr/local/lib/libarmadillo.dylib) was built for newer macOS version (11.0) than being linked (10.14)
But its not fixed rn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I overlooked the details of these lines of code.
But why is there \ after 11?
And better regex shouldn't be 11.([0-9]+)((.[0-9]+)*)
As version can be of form 11.V only.
I will change 11 to 10 back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

([0-9]+)(?:.([0-9]+).)

Comment on lines +37 to +38
inputWidth(inputHeight),
inSize(0)
Copy link
Member

Choose a reason for hiding this comment

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

I would open another PR just for this one, and remove it from here, this will help us better keep tracking what this PR is doing on the CI fix, since this is not related directly to the CI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just warning removal actually. You can do whatever you want to do.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Unless I'm missing something it looks like you fixed the build issue for now by using an older version? Which I think is fine.

@shubham1206agra
Copy link
Contributor Author

I will close this PR. And open separate ones once correction is done.

@shubham1206agra
Copy link
Contributor Author

@shrit Please see #3124 , #3125 , #3126

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

Successfully merging this pull request may close these issues.

None yet

3 participants