Include object store definitions

[shaunrd0]

I was running some tests through rest and after rebuilding on dev I ran into issues where HAVE_S3=false in core when TILEDB_S3=ON and TILEDB_VCPKG=OFF in cmake, which results in hitting this error when querying an S3 array:

https://github.com/TileDB-Inc/TileDB/blob/dev/tiledb/sm/filesystem/vfs.cc#L185

S3 CI is passing with vcpkg enabled so I added this back with a check for NOT TILEDB_VCPKG, which seems to have fixed the issue for me.

---

TYPE: BUG
DESC: Include object store definitions when TILEDB_VCPKG=OFF

[ihnorton]

Thanks, fixed an issue I was just hitting locally

[davisp]

This looks not quite right, though I see @ihnorton already removed the TILEDB_VCPKG check. The only other place that's currently used is on line 383 of the same file although its for the TILEDB_CORE_OBJECTS instead of TILEDB_CORE_OBJECTS_ILIB. Might just be we need to append the _ILIB on 383.

[shaunrd0]

The _ILIB library isn't created until just above the line I added here. I could move creation and linking up to 383 if that's what you're thinking? Or would deleting 383 and leaving creation where it is be the same change? I'm not sure how this impacts the vcpkg work (which is awesome btw 🎉) I just ran into this yesterday and after looking at diffs from #4055 I thought maybe this was accidentally removed for non-vcpkg builds.

[davisp]

Huh, I definitely see your point in that it certainly looks like we we're a bit too aggressive removing that line in #4055. I don't remember the exact motivation there. The CMake failures on this PR are probably due to the removal of line 1101 in tiledb/CMakeLists.txt [1] as well. For now I'm fine just putting them both back as they were and not getting carried away attempting to rework this bit of the build system in response to a bug fix.

[1] https://github.com/TileDB-Inc/TileDB/pull/4055/files#diff-64e075a38340d9989b5b947d543f08464f279b09f838b898dc5bf5a887b64603L1101

A quick note to self for when we do get around to doing some refactoring on the CMake definitions. It seems like we're confusing a number of things here by installing TILEDB_CORE_OBJECTS_ILIB as a target (which I assume makes it available for people to re-use linking against libtiledb for other projects. We should create two interface libraries, one which I'd call something similar like TILEDB_CORE_ILIB which is basically equivalent to TILEDB_CORE_OBJECTS_ILIB that we use for compiling all of the code that goes into libtiledb. Then a second distinct interface (called something like LIBTILEDB_ILIB) that is used by all the code that links against libtiledb (including all the parts of our build system that do this, like the tools, examples, tiledb_unit and friends, and so on) and then LIBTILEDB_ILIB is the only target insalled for use by downstream projects. A simple obvious reason for why we need two is that depending on static vs dynamic linking options. Or even just our own internal compiler flags like -DTILEDB_SERIALIZATION which shouldn't be abused for things like feature probing.

[ihnorton]

It seems like we're confusing a number of things [...] TILEDB_CORE_OBJECTS_ILIB

We can simplify by getting rid of this target completely now that we require CMake > 3.12 -- see the note about compatibility workaround near the defining add_library. (and, better than that, eventually using all of the individual library targets rather than listing/compiling each source file twice)