Skip to content

Fix CI#395

Merged
Snaipe merged 10 commits intoSnaipe:bleedingfrom
MrAnno:fix-ci
Dec 17, 2021
Merged

Fix CI#395
Snaipe merged 10 commits intoSnaipe:bleedingfrom
MrAnno:fix-ci

Conversation

@MrAnno
Copy link
Collaborator

@MrAnno MrAnno commented Nov 11, 2021

This PR fixes Cirrus CI build and test jobs on macOS, Windows, and FreeBSD.

I've also had a quick look at GitHub Actions, it seems to be a stable/native/fast CI alternative, which works really well with GitHub.
Unfortunately, FreeBSD is not officially available there.

Depends on Snaipe/BoxFort#31
Depends on Snaipe/BoxFort#32

Resolves the linking issue in #391
Fixes #377

@MrAnno MrAnno changed the title Fix CI [WIP] Fix CI Nov 11, 2021
@MrAnno
Copy link
Collaborator Author

MrAnno commented Nov 11, 2021

The macOS job is failing because certain test binaries have no permissions to call mprotect(PROT_READ | PROT_WRITE | PROT_EXEC) for injecting Boxfort's trampoline.

@MrAnno
Copy link
Collaborator Author

MrAnno commented Nov 11, 2021

Yep, it only works when libcriterion is dynamically linked to the test.
I hope it's not something similar to "Arbitrary Code Guard".

Update:
Unfortunately, that's a protection in macOS (maxSegProtection).
https://stackoverflow.com/questions/60654834/using-mprotect-to-make-text-segment-writable-on-macos

I was able to bypass this using vm_protect() with VM_PROT_COPY.

@MrAnno MrAnno force-pushed the fix-ci branch 2 times, most recently from 8a97055 to 5ce5106 Compare November 17, 2021 14:49
@MrAnno
Copy link
Collaborator Author

MrAnno commented Nov 21, 2021

cram adds an environment variable called TESTSHELL to each test.
Unfortunately, the value is encoded with fsencode, which produces a bytes object on Windows, but Python on Windows does not support non-string environment:

'TESTSHELL': b'C:\\Program Files\\Git\\usr\\bin\\sh.EXE'

File "C:\Python\lib\subprocess.py", line 1435, in _execute_child
    hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
TypeError: environment can only contain strings

I'm afraid I can not fix this without modifying the source of cram.

https://github.com/brodie/cram/blob/59c164dfa6cbe4845aad2c958e77695073d5e802/cram/_main.py#L22
https://github.com/brodie/cram/blob/59c164dfa6cbe4845aad2c958e77695073d5e802/cram/_test.py#L110

See also: aiiie/cram#24

@MrAnno MrAnno changed the title [WIP] Fix CI Fix CI Nov 21, 2021
@MrAnno MrAnno marked this pull request as ready for review November 21, 2021 15:31
@Snaipe
Copy link
Owner

Snaipe commented Nov 22, 2021

I'm not opposed in keeping a couple of patches in the ci/ directory and applying them during the windows build. In fact, this used to be the case for some python2/3 compatibility changes on cram some time ago.

@MrAnno
Copy link
Collaborator Author

MrAnno commented Nov 22, 2021

Okay, I will give it a try. Thank you, @Snaipe.

@MrAnno MrAnno force-pushed the fix-ci branch 7 times, most recently from 4a9d3e9 to f4ef92b Compare November 23, 2021 00:11
@MrAnno
Copy link
Collaborator Author

MrAnno commented Nov 23, 2021

Yay, it works!

With Snaipe/BoxFort#32, all tests passed.

@MrAnno MrAnno marked this pull request as draft November 23, 2021 00:25
@kasperk81
Copy link

cram is abandadoned for 5+ years now https://github.com/brodie/cram (last commit from 2016).
maybe switch to exactly https://github.com/emilkarlen/exactly#id1 @emilkarlen can probably comment how it compares with cram.

@emilkarlen
Copy link

Hi,
thanks for interest in exactly! I will take a closer look at what your tests look like soon - tonight or tomorrow (and also at cram, which is new to me).

Copy link
Owner

@Snaipe Snaipe left a comment

Choose a reason for hiding this comment

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

We can open a separate issue for evaluating what it'd take to switch from cram; for now, this overall looks good to me.

This is an attempt to fix cram tests in non-POSIX-compliant environment.
This commit contains a patch for cram to fix an environment encoding issue:
TypeError: environment can only contain strings

Cram tests should be executed by `meson devenv`, because meson does not
seem to add rpath entries on Windows.
@MrAnno MrAnno marked this pull request as ready for review November 29, 2021 09:18
@MrAnno
Copy link
Collaborator Author

MrAnno commented Nov 29, 2021

@Snaipe Thank you for reviewing my PR.

I've updated the BoxFort subproject revision after you merged Snaipe/BoxFort#32.

@emilkarlen
Copy link

I am looking at Exactly vs Cram.
It took some time to get Criterion building.

I've started to implement the cram tests using Exactly: https://github.com/emilkarlen/Criterion-exactly

@emilkarlen
Copy link

I have added Exactly counterparts of the Cram tests:
emilkarlen@8778dca

if is of interest.

@Snaipe
Copy link
Owner

Snaipe commented Dec 17, 2021

@MrAnno Thanks for the changes, LGTM.

@emilkarlen Would you mind opening a PR with the changes on your fork? This will be a better place for discussing anything related to the topic of moving away from Cram.

@Snaipe Snaipe merged commit a91a69f into Snaipe:bleeding Dec 17, 2021
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.

criterion: ftbfs with GCC-11

4 participants