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

Jump to bottom

CRuntime_Musl: More fixes for time64 #3384

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

CRuntime_Musl: More fixes for time64 #3384

wants to merge 1 commit into from

Conversation

omerfirmak
Copy link
Contributor

@omerfirmak omerfirmak commented Feb 26, 2021 

The original PR (#3275) missed quite a few spots and conversions,
which led to the build on Alpine Linux failing with Aithmetic Exception
on core.time module constructor.
Links to the two offending commits are included.
For further issues / investigation, search for 'time64' in the git repository.

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 26, 2021

Thanks for your pull request and interest in making D better, @omerfirmak! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + druntime#3384"

ibuclaw
ibuclaw reviewed Feb 27, 2021
View changes
src/core/stdc/time.d
public import core.sys.windows.stdc.time;
// This enum is defined only for Posix, this file is the only one
// needing it in `core.stdc`.
private enum CRuntime_Musl_Needs_Time64_Compat_Layer = false;
Copy link
Member

@ibuclaw ibuclaw Feb 27, 2021

Choose a reason for hiding this comment

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

Perhaps all function declarations present here should be moved to core.sys.<platform>.stdc.time then.

src/core/sys/linux/sys/socket.d
{
enum SO_TIMESTAMP = 63;
enum SO_TIMESTAMPNS = 64;
enum SO_TIMESTAMPING = 65;
Copy link
Member

@ibuclaw ibuclaw Feb 27, 2021

Choose a reason for hiding this comment

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

Wait, so now there is backwards compatibility with Musl, but not older Linux kernels?

kinke
kinke reviewed Mar 1, 2021
View changes
src/core/stdc/time.d Show resolved Hide resolved
@Geod24 @omerfirmak
CRuntime_Musl: More fixes for time64
9799355 
The original PR (dlang#3275) missed quite a few spots and conversions,
which led to the build on Alpine Linux failing with Aithmetic Exception
on core.time module constructor.
Links to the two offending commits are included.
For further issues / investigation, search for 'time64' in the git repository.

Co-Authored-By: Ömer Faruk IRMAK <omerfirmak@gmail.com>
@RazvanN7
Copy link
Contributor

RazvanN7 commented Mar 2, 2021 •
edited

What is the relationship between this PR and #3384 ? It seems that some changes are overlapping. Why not make a single PR with multiple commits?

@kinke
Copy link
Contributor

kinke commented Mar 2, 2021

This targets stable, #3383 dmd-cxx (presumably for bootstrapping).

@RazvanN7
Copy link
Contributor

RazvanN7 commented Mar 12, 2021

@kinke Ok to merge this?

@ibuclaw ibuclaw self-assigned this Jun 6, 2021
@dlang-bot dlang-bot added the Needs Rebase label Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibuclaw ibuclaw

@kinke kinke

@andralex andralex

@Burgos Burgos

@CyberShadow CyberShadow

@MartinNowak MartinNowak

@schveiguy schveiguy

@wilzbach wilzbach

At least 1 approving review is required to merge this pull request.

Assignees

@ibuclaw ibuclaw

Labels
Needs Rebase stable-stalled stalled
Projects
None yet
Milestone
No milestone
6 participants
@omerfirmak @dlang-bot @RazvanN7 @kinke @ibuclaw @Geod24