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

Make runnable on FreeBSD #417

Merged
merged 2 commits into from Feb 14, 2019

Conversation

Projects
None yet
2 participants
@melak
Copy link
Contributor

commented Feb 11, 2019

No description provided.

@ubruhin

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

Hi @melak,

Thanks for the patch! It looks like the code you added is exactly the same as on Unix, so probably we could just change #elif defined(Q_OS_UNIX) to #elif defined(Q_OS_UNIX) || defined(Q_OS_FREEBSD), couldn't we? That would be great to avoid code duplication.

And was this the only patch needed to get it on FreeBSD compiling? Aren't there other preprocessor directives failing too?

@melak

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Might look like so, but it is not. It's using /proc/pid/file, not /proc/pid/exe; it also doesn't do Linux-specific postprocessing on the resolved link.

It would actually be more prudent to fix the original block by guarding it with Q_OS_LINUX, not Q_OS_UNIX, and just error out at compile-time for an unknown platform. The code block currently guarded by Q_OS_UNIX is actually rather quite Linux-specific.

@ubruhin

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Might look like so, but it is not. It's using /proc/pid/file, not /proc/pid/exe;

Oops you're right, I didn't notice that ;)

It would actually be more prudent to fix the original block by guarding it with Q_OS_LINUX, not Q_OS_UNIX, and just error out at compile-time for an unknown platform. The code block currently guarded by Q_OS_UNIX is actually rather quite Linux-specific.

Hmm good point. Is your code block actually only valid for FreeBSD, or maybe even for all Unix derivatives? If it's valid for all Unix systems, it might make sense to change it to following:

#if defined(Q_OS_OSX)
  // Mac OS X
#elif defined(Q_OS_LINUX)
  // Linux
#elif defined(Q_OS_UNIX)
  // Unix
#elif defined(Q_OS_WIN32) || defined(Q_OS_WIN64)
  // Windows
#endif
@melak

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

It almost certainly isn't valid for anything else. (This is one of those bobs and bits that's not portable, oftentimes even across systems with shared roots.)

Point is, it's not worth overthinking this. Current code certainly doesn't work on anything but Linux. There's no point in overthinking this - if you make these fragments compile on their (well-defined) systems and not compile on "a general Unix" (it's a futile effort), people who want to run them on other systems will submit patches. If they don't, you didn't need it to run on anything else to begin with.

@ubruhin

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

OK I see... So would you like to change Q_OS_UNIX to Q_OS_LINUX? If yes, feel free to update this PR accordingly. Otherwise I'll just merge it as-is.

@melak

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

Updated.

@ubruhin

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

Thanks! 👍

@ubruhin ubruhin merged commit ed89297 into LibrePCB:master Feb 14, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ubruhin added a commit that referenced this pull request Mar 17, 2019

Merge pull request #417 from melak/freebsd
Make runnable on FreeBSD
(cherry picked from commit ed89297)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.