Skip to content

Simplified detection of macOS for install_name_tool#146

Merged
eddelbuettel merged 1 commit intomasterfrom
feature/update_makevars
Jul 28, 2020
Merged

Simplified detection of macOS for install_name_tool#146
eddelbuettel merged 1 commit intomasterfrom
feature/update_makevars

Conversation

@eddelbuettel
Copy link
Contributor

The previous addition to protect use of install_name_tool upset Conda, so it was changed.

Which upsets R CMD check because it used ifneq which, according to the gospel of CRAN, is non-portable.

So I changed it again, borrowing the construct from data.table which, as I checked, has a conda build so the new double test appears to hold water. Actual call unchanged, all we loose is the "non-portable if" and an echo statement we do not really need.

@Shelnutt2
Copy link
Member

In addition to the uname checks, I believe need need a check for "Are we linking against the downloaded library". The problem with conda is we don't link against the downloaded library but a system install and that is where the use of install_name_tool fails because the files don't exist since they were not downloaded since the system install of TileDB was detected.

@eddelbuettel
Copy link
Contributor Author

In addition to the uname checks, I believe need need a check for "Are we linking against the downloaded library". The problem with conda is we don't link against the downloaded library but a system install and that is where the use of install_name_tool fails because the files don't exist since they were not downloaded since the system install of TileDB was detected.

So just add a test -f library file layer ?

I was leaning towards moving the "shall we do this?" question into configure.ac and have is either drop-in or not the code chunk via a standard @var@ filled by configure. But then I remembered the much simpler setup by data.table which, according to git blame, has been there since 2016.

@eddelbuettel
Copy link
Contributor Author

And/or we add a third logical condition that will be TRUE for normal builds and FALSE for Conda. Filled in by configure.

And/or we use an env.var for conda and if set skip this. How would I detect a conda build?

@Shelnutt2
Copy link
Member

And/or we add a third logical condition that will be TRUE for normal builds and FALSE for Conda. Filled in by configure.

And/or we use an env.var for conda and if set skip this. How would I detect a conda build?

I'm much more in favor of a generic solution. Conda is just one env where we'll have a preexisting library. Seems like development would be another common use case were we want to avoid trying to run install_name_tool if we are linking against a system install.

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Jul 28, 2020

We already cover preexisting libraries, and allow building, and only download where needed. So we already are more general:

edd@rob:~/git/tiledb-r(master)$ ./configure --help                                                       
`configure' configures TileDB-R 0.7.1 to adapt to many kinds of systems.
[...]
  --enable-building       build TileDB library locally instead of downloading                            
                          prebuilt     
[...]
  --with-tiledb=PREFIX    path to installed built of TileDB  

and of course if the library is in a standard location it "just works" without any options. Which is, in essence, all my builds.

with a tip of the hat to to data.table and an additional file-present test
@eddelbuettel eddelbuettel force-pushed the feature/update_makevars branch from 25903c5 to ca5d0a5 Compare July 28, 2020 22:10
@eddelbuettel
Copy link
Contributor Author

Helpful discussion on and off-repo --- I just updated the test to check for (both) files before potentially hitting them with install_name_tool while keeping the initial two tests.

@eddelbuettel eddelbuettel merged commit 939cb0b into master Jul 28, 2020
@eddelbuettel eddelbuettel deleted the feature/update_makevars branch July 28, 2020 23:16
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