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

Update mkfile #318

Merged
merged 1 commit into from
Apr 13, 2024
Merged

Update mkfile #318

merged 1 commit into from
Apr 13, 2024

Conversation

staalmannen
Copy link
Contributor

Add -D_BSD_SOURCE to successfully include <sys/time.h> in some files that have changed.

@Bill-Gray
Copy link
Owner

Hi Jens,

Just to make sure I'm understanding this : the problem is that, for Plan 9, we ought to use gettimeofday() instead of falling through to ftime() or clock_gettime(). So... we know we'll have gettimeofday() on Plan 9; do we know that everything else implied by -D_BSD_SOURCE will be available?

If we know it'll all be available, then we can just pull your change. If not, we should do something more specific. One possibility would be to modify this line to add something like

#if defined( _DEFAULT_SOURCE) || defined( _BSD_SOURCE) || defined( _PLAN_9)

(or similar macro specifying we're on Plan 9).

Alternatively, we could add a very specific -DGETTIMEOFDAY_AVAILABLE compile flag (replacing the -D_BSD_SOURCE one you've added).

Thanks! -- Bill

@staalmannen
Copy link
Contributor Author

Hi. I have -D_BSD_EXTENSION defined so most stuff should be covered. I have not done a new test of the library against demos yet, but the library built just fine without any complaints. If the build would have assumed some other functionality under _BSD_SOURCE I guess it would already have been during compilation or linking?

Just after I made that change, I noticed that you now have term.h and some terminfo stub stuff so I changed again in a 2nd commit to the same pull request.

@staalmannen
Copy link
Contributor Author

but indeed we have gettimeofday in sys/time.h

https://github.com/rdbyk/9front/blob/master/sys/include/ape/sys/time.h

@Bill-Gray
Copy link
Owner

I think that at present, you are correct in thinking that #defining _BSD_SOURCE causes no issues for us. Some of the discussion here (use of _BSD_SOURCE causes 4.3 BSD definitions to take place over POSIX ones; you need to use a "special BSD compatibility library") makes me a little nervous; it sounds as if we could run into problems at some point in the future. I read it as saying "you can #define _BSD_SOURCE if you have some old BSD code and want to compile it without POSIX-izing it".

However... further inspection makes me think that #defining _BSD_EXTENSION is just a Plan 9 specific way of saying, "we have BSD extension functions". So I think the line above could be revised to read

#if defined( _DEFAULT_SOURCE) || defined( _BSD_SOURCE) || defined(_BSD_EXTENSION)

and we'd get the desired behavior without any knock-on effects.

Thanks for taking this on. I've had thoughts for some time of setting up Plan 9 and learning a bit about it (seems to have some interesting ideas in it), but haven't actually done so. So I could have broken things in Plan 9 PDCursesMod without realizing it... hope that hasn't happened!

@staalmannen
Copy link
Contributor Author

Sure that sounds great to me. Also the option of a -DGETTIMEOFDAY_AVAILABLE would be perfectly fine for me as a more granular definition if we indeed expect more general divergence based on _BSD_SOURCE. From I read about the above discussion that had to do with the implementation in glibc, which of course is not relevant for Plan9.

I can change the pull request to utilize -DGETTIMEOFDAY_AVAILABLE if you want

@GitMensch
Copy link
Collaborator

GitMensch commented Apr 8, 2024 via email

Copy link
Collaborator

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

LGTM - can you please squash those commits into one?

Add -D_BSD_SOURCE to successfully include <sys/time.h> in some files that have changed.

Update mkfile - add term.h

terminfo / term.h now installed

add -DHAVE_GETTIMEOFDAY instead of -D_BSD_SOURCE

added some new demo tests to mkfile
@Bill-Gray Bill-Gray merged commit 1e8472c into Bill-Gray:master Apr 13, 2024
4 checks passed
@Bill-Gray
Copy link
Owner

I've stayed away from the HAVE_* #defines, because they get used elsewhere and I had concerns about conflicts. However, I think that would only happen if we re-defined those macros within our code, and that's not what you're doing here. So I can't see it causing trouble, so... merged (with some edits to the commit message). Thank you!

@GitMensch
Copy link
Collaborator

I've stayed away from the HAVE_* #defines, because they get used elsewhere and I had concerns about conflicts. However, I think that would only happen if we re-defined those macros within our code, and that's not what you're doing here. So I can't see it causing trouble, so... merged (with some edits to the commit message). Thank you!

t is totally fine (and correct) to use the HAVE_ defines per its standard: HAVE_functionname and HAVE_HEADER_H.
In this case the makefile is specific to plan9 where we know the function exists and works with the compiler that is included in it - so all fine.

@staalmannen staalmannen deleted the patch-1 branch April 16, 2024 10:05
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.

3 participants