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

Common/log.h: time(2) needs <time.h> #250

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jacmet
Copy link

@jacmet jacmet commented Sep 25, 2018

Otherwise the build fails with:

In file included from ./linux/os.h:28:0,
from Common/DtaOptions.cpp:20:
./Common/log.h: In function ‘std::__cxx11::string NowTime()’:
./Common/log.h:349:12: error: ‘time’ was not declared in this scope
time(&t);

Signed-off-by: Peter Korsgaard peter@korsgaard.com

Otherwise the build fails with:

In file included from ./linux/os.h:28:0,
                 from Common/DtaOptions.cpp:20:
./Common/log.h: In function ‘std::__cxx11::string NowTime()’:
./Common/log.h:349:12: error: ‘time’ was not declared in this scope
     time(&t);

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
@oom-is
Copy link

oom-is commented Oct 15, 2019

Could you please confirm that this patch is still needed and on what platform(s)?

I've compiled current sedutil source against the buildroot2 2019.02.6 (LTS) target on Ubuntu 18.04.3 LTS (using a local.mk file to override source location) and I did not need to make this change. If it's needed only for specific build platforms, I'd rather document that need than make the change globally.

@jacmet
Copy link
Author

jacmet commented Oct 15, 2019

It naturally depends on the toolchain you are using (E.G. you may get lucky and <time.h> gets included by something else, but 'man 2 time' clearly documents that the prototype is defined in time.h

@jacmet
Copy link
Author

jacmet commented Oct 15, 2019

@oom-is what exactly do you mean with "is still needed"? I don't see any changes to this repo since the 1.15.1 release from 2017?

@oom-is
Copy link

oom-is commented Oct 16, 2019

[Side note: Thanks very much for Buildroot!]

Sorry I wasn't at all clear. I opened a pull request #306 that updates sedutil to a newer Buildroot LTS target (2019.02.6) and newer Linux LTS kernel (4.14.146), and since sedutil was included in Buildroot as of 2018.11.1 that meant I had to override the Buildroot-specific patches and use a local source tree.

When building that set of source+toolchain under Buildroot without your time.h patch, but otherwise with "stock" sedutil 1.15.1 source, everything compiled successfully as far as my testing has shown. Build host was Ubuntu 18.04.3 LTS.

At the same time, your patch is technically correct as you've stated and Does No Harm - so the "curated" version that I'm putting together for $work will include your patch.

I've had no luck reaching anyone at DTA even for questions about licensing of their native Windows GUI (SEDcontrol), so it appears there's little chance that any of these changes will be adopted into the official source tree any time soon.

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.

None yet

2 participants