Skip to content

Protect windows file with #ifdef #333

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.

Sign up for GitHub

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

Closed
eddelbuettel wants to merge 1 commit into from
Closed

Protect windows file with #ifdef #333

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/time_zone_name_win.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#if defined(_WIN32)
Copy link
Contributor

@devbww devbww Dec 29, 2025

Choose a reason for hiding this comment

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

Hi Dirk,

I'm not diametrically opposed to this.

However, I was undoubtedly the one who requested that platform-specific code be preferably treated using the build system and/or file system rather than by conditional compilation, and it would be nice if we continued, and even extended that trend (* see below) instead of reversing it.

I took a quick look at your repository, but couldn't really see how things are built. It appears that R supports Windows though, so my question to you is what your build systems can do to support platform-specific source files (similar to what CCTZ does with bazel and cmake), rather than just (apparently) trying to build everything. I proffer that would be a more desirable path forward.

(*) I probably didn't go far enough in my reviews of this platform-specific code. It would be better if there was a platform-independent GetLocalTimeZoneName() interface that was implemented in platform-specific files, rather than the mismatch of files and #ifdefs that we currently have. It would also be nicer (methinks) if we used a "<platform>/" prefix for such implementations instead of a "_<platform>" suffix.

Copy link
Author

@eddelbuettel eddelbuettel Dec 29, 2025

Choose a reason for hiding this comment

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

Hi there,

Yes the key difference is that cmake allows you to modify the set of files. R, by default, uses one fixed tar.gz and then two different Makefiles snippets called src/Makevars and src/Makevars.win: one for Unix systems including macOS, and one for Windows.

So we could play games here and have a (shell) script configure copy the two files in.

Or we could play a different games and explicitly list the files built -- and have those differ between the Windows and non-Windows builds. But ... why? Adding a #define is clean, simple and well understood. R has a (by default) build "simply": it can work without a Makevars, and implicit make rules work. All files found are compiled and linked into the dynamically loadable extension a package brings. So my preference would be to keep it simple, following best and time-honoured practices of 'least surprises'.

Regarding your last paragraph, we could go that route and always have the stub function present and only on Windows call out to that platform specific code. I have written and published R packages at CRAN that do that, see for example RcppAPT which really only can work where libapt-dev exists and is empty even on, say, Fedora. See https://github.com/eddelbuettel/rcppapt/tree/master/src, the #define is set from configure.

Anyway, most importantly I would prefer for us to have this be 'low touch' -- I have happily provided your very nice cctz library here for a decade here and mostly got by without crazy acrobatics (and as I recall on occassional reported a minor need for a specific compiler tweak back to you even though I apparently never forked this).

Copy link
Author

@eddelbuettel eddelbuettel Dec 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I guess I could enumerate the files as in

SOURCES = \
   civil_time_detail.cc \
   time_zone_fixed.cc \
   time_zone_format.cc \
   time_zone_if.cc \
   time_zone_impl.cc \
   time_zone_info.cc \
   time_zone_libc.cc \
   time_zone_lookup.cc \
   time_zone_posix.cc \
   zone_info_source.cc 

OBJECTS = $(SOURCES:.cc=.o)

$(SHLIB): $(OBJECTS)

and that satisfy the R standards ("no GNU make extension or else declare a GNU make dependency") and for Windows I could add the one file. A bit tedious as I know need to maintain a master list but you could probably (and rightly) argue that I already did that by excluding the test and benchmark files.

(For context, R takes the 'snippet' that is src/Makevars and creates / forms a full Makefile around it. That Makefile is ephemeral and disappears.)

Copy link
Author

@eddelbuettel eddelbuettel Dec 29, 2025

Choose a reason for hiding this comment

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

Ok, I have the alternate approach working (as validated by uploading to the (hosted-by-the-R-Project) win-builder along with local testing under Linux.

I still find the two-line (#if defined, and #endif) change simpler but your project, your rules. I reserve myself the same rights for my projects so naturally am playing along here too.


#include "time_zone_name_win.h"

#if !defined(NOMINMAX)
Expand Down Expand Up @@ -175,3 +177,5 @@ std::string GetWindowsLocalTimeZone() {
}

} // namespace cctz

#endif