Skip to content

Conversation

@tonchis
Copy link
Contributor

@tonchis tonchis commented Sep 6, 2022

No description provided.

@tonchis tonchis requested a review from jemc September 6, 2022 13:11
@tonchis tonchis self-assigned this Sep 6, 2022
@tonchis
Copy link
Contributor Author

tonchis commented Sep 6, 2022

@jemc I made this a draft because I see there are many tasks in the CI failing and didn't wanna merge with a broken pipeline. The ubuntu and macOS builds are passing, but since this is a BSD specific change it'd be nice to see it passing on such machine.

@mneumann
Copy link
Contributor

mneumann commented Sep 7, 2022

love it!

@jemc
Copy link
Contributor

jemc commented Sep 7, 2022

Yes, looks like we have multiple CI issues to address, both with an Alpine version upgrade and a Crystal version upgrade.

None of them are likely to be related to this PR.

I will aim to make time today to get the pipeline green again.

@jemc
Copy link
Contributor

jemc commented Sep 7, 2022

I have a PR that should hopefully resolve the breakages for the existing CI jobs.

I agree with you that it would be nice to have FreeBSD CI working.

In particular, we won't be able to run FreeBSD CI for any of the libraries unless/until we get it working here for the main Savi build.

I did some work on a FreeBSD CI job a while back, but it's currently commented out because it's not working. I believe the key obstacle was that I couldn't get the latest version of Crystal installed on the FreeBSD CI runner - the existing "FreeBSD port" was pinned to an older version.

@mneumann (who uses FreeBSD) may have ideas about how to get past that obstacle.

@jemc
Copy link
Contributor

jemc commented Sep 7, 2022

Looking at https://www.freshports.org/lang/crystal/ I see that it looks like it includes the latest Crystal version (1.5.0) so it may be doable now. I'll create another branch to try to get FreeBSD CI up and running.

@jemc
Copy link
Contributor

jemc commented Sep 7, 2022

The above mentioned PR is merged, and here's a working PR to add a FreeBSD CI job: #340

:: Returns `True` if the target platform uses a FreeBSD operating system.
:const is_freebsd Bool: compiler intrinsic

:: Returns `True` if the target platform uses a OpenBSD operating system.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a tiny stupid English nitpick:

Suggested change
:: Returns `True` if the target platform uses a OpenBSD operating system.
:: Returns `True` if the target platform uses an OpenBSD operating system.

Comment on lines +13 to +18
if Platform.is_bsd (assert: (
Platform.is_freebsd ||
Platform.is_openbsd ||
Platform.is_netbsd ||
Platform.is_dragonfly
))
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that of these four platforms are mutually exclusive, right?

If so, then I think a test like the following would be more robust, and cover more failure cases:

Suggested change
if Platform.is_bsd (assert: (
Platform.is_freebsd ||
Platform.is_openbsd ||
Platform.is_netbsd ||
Platform.is_dragonfly
))
is_bsd U8 = if Platform.is_bsd (1 | 0)
assert: is_bsd == U8[0] +
if Platform.is_freebsd (1 | 0) +
if Platform.is_openbsd (1 | 0) +
if Platform.is_netbsd (1 | 0) +
if Platform.is_dragonfly (1 | 0)

That is, this tests that exactly one of those specific BSD platforms is true if and only if the overall platform designation is BSD - and if it's not BSD, then none of them are true.

@jemc jemc force-pushed the enhance/platform-freebsd-openbsd-netbsd-dragonfly branch from 009e2ed to 84eb78c Compare September 7, 2022 20:20
@jemc
Copy link
Contributor

jemc commented Sep 7, 2022

I've rebased this PR on latest main, which should resolve the CI failures.

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.

4 participants