Skip to content

Replace -DLINUX/-DLINUX_ with __linux__ #1296

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

Closed
wants to merge 3 commits into from
Closed

Replace -DLINUX/-DLINUX_ with __linux__ #1296

wants to merge 3 commits into from

Conversation

mattst88
Copy link
Contributor

No description provided.

@dvrogozh
Copy link
Contributor

@mattst88 : while that's a correct thing to do per-se, can you, please, provide your side motivation why you are doing this change? is there any functional or build time issue standing behind?

@mattst88
Copy link
Contributor Author

@mattst88 : while that's a correct thing to do per-se, can you, please, provide your side motivation why you are doing this change? is there any functional or build time issue standing behind?

I don't really have a side motivation; I just started helping to package this software in Gentoo Linux, glanced through the GitHub issues, and thought I'd see how easy or difficult it was to get contributions accepted from outside the company.

I guess if there's a side motivation, it's to work towards standardizing the build system to make it easier for packagers.

Copy link
Contributor

@dvrogozh dvrogozh left a comment

Choose a reason for hiding this comment

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

@mattst88 : thank you, this aligns to my current understanding.

s/LINUX_/linux/ are all good. But changes where you remove self-defined macros are a little tricky because there is internal code which is not yet available in this public repo and it also uses these macros. And we have a sync in process to avoid code divergence. This sync eventually will be impacted. Thus, this change needs to be done in few stages:

  1. Make all changes possible in this PR which won't impact internal flow
  2. Someone internally need fix the internal code
  3. After that cmake level change can be done

Thus, may I ask you to remove some cmake level changes to make this PR easily mergeable? I'll mark them in inline comments.

@dvrogozh
Copy link
Contributor

@jbeich : can you, please, evaluate how FreeBSD will be impacted by this change?

@jbeich
Copy link

jbeich commented Nov 29, 2021

At #819 DragonFly, FreeBSD, NetBSD, OpenBSD and, tentively, Solarish (OpenIndiana) all currently use -DLINUX (explicitly defined) in order to avoid patch churn. With __linux__ (peredefined macro) to support all those Unix-like platform one would need to replace __linux__ in the code with !_WIN32 (anything non-Windows), use __unix__ (predefined on Linux and BSDs but not macOS) or explicitly check for __DragonFly__, __FreeBSD__, __NetBSD__, __OpenBSD__ and __sun. Note, defining __linux__ on BSDs breaks system headers e.g., 7bf1b00.

Downstream #819 already uses as cherry-picks (FreeBSD, DragonFly) for years and also as local copies (OpenBSD). So, merging this PR may create more work (rebases and monitoring new instances) depending on how frequently __linux__ lines are touched between releases.

@mattst88
Copy link
Contributor Author

I would be happy to rebase/redo this on top of your patch series, if Intel would accept yours. It's approaching its second birthday however...

@mattst88
Copy link
Contributor Author

mattst88 commented Dec 14, 2021

Two weeks have passed.

Could someone explain what the actual process is for accepting PRs from Github? I see tags like 'verifying' which indicate to me that something is happening behind the scenes.

Is there someone upstream who is interested and able to merge external contributions?

@dvrogozh
Copy link
Contributor

PR should pass review and get approval. Then someone on Intel side should trigger internal ci for the PR, if it will pass it will be merged. "verifying" label is to mark PRs which got picked for this internal ci.

In case of this PR we got #1296 (comment) which highlights that we can't take this patch as is without taking care of freebsd. I understand that we don't support freebsd right now w/o additional patches, but I don't want to make things worse than they are.

@mattst88
Copy link
Contributor Author

Obsoleted by the first patch in #1785

@mattst88 mattst88 closed this Nov 20, 2024
@mattst88 mattst88 deleted the DLINUX branch November 20, 2024 14: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