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

Clarify needed settings for pre-built binaries. #361

Merged
merged 3 commits into from
Mar 3, 2020

Conversation

najamelan
Copy link
Contributor

Closes #359

I think it would be great if tarpaulin had CI itself to verify that all
recommended installation procedures actually lead to working setups.

Unfortunately I don't have the time to set that up for you.

Closes xd009642#359

I think it would be great if tarpaulin had CI itself to verify that all
recommended installation procedures actually lead to working setups.

Unfortunately I don't have the time to set that up for you.
@xd009642
Copy link
Owner

xd009642 commented Mar 1, 2020

So I think this can be more succinct if it doesn't mention libssl-dev, after-all that is in the example travis.yml so users should always add it.

Also I realised why the prebuilt doesn't work on trusty anymore and it's because I switched to github actions and ubuntu-latest on GA is bionic. So maybe adapt the note to say that the prebuilt binaries are built on ubuntu-latest with github actions so the ubuntu distribution for travis should match if using them which at the current time is bionic

@najamelan
Copy link
Contributor Author

Ok, but then the example .travis.yml also has:

  • dist: trusty
  • sudo: required -> not needed in my tests at least with bionic
  • after_success -> which means if the code coverage step fails, your ci result will still be green, making problems go undetected?
  • cargo install cargo-tarpaulin -> so when the text below the example says alternatively it's not quite clear what to include for the pre-built binary.
  • it doesn't have pkg-config, cmake, zlib1g-dev in the package list. So do you need these to compile from source or not?

What do you want to do about all of these issues? In any case, "allow edits from maintainers is on", don't hold back, this is your project after all.

@xd009642
Copy link
Owner

xd009642 commented Mar 2, 2020

So dist: trusty could either delete or replace with dist: bionic or add a note saying it's known to work with both versions. I remember there was an issue with one of the travis ubuntu versions where it didn't work but that was so long ago it might not be an issue.

sudo required seemed to be required by a lot of people, but maybe that depends on which ubuntu version: see #77 for reference

after_success was added because some people complained if their tests and build pass tarpaulin failing to run on the project shouldn't fail CI as it's more a tarpaulin issue if it can't build/run a project

the alternatively bit it was intended that only the install command should have to change. And I'm sure with travis it preinstalls pkg-config, cmake, zlib1g-dev hence why the example config just has libssl-dev.

I'll have a think about what I think is best probably need to personally take some time and figure out how much of the readme needs updating based on the current state of things

@xd009642
Copy link
Owner

xd009642 commented Mar 2, 2020

I'll try to get past some initial issues just to merge this PR tonight though as some things may require more time to decide on. And obviously if you have any opinions or thoughts I'm always open to suggestions 👍

@najamelan
Copy link
Contributor Author

Sounds great. If the readme is clear, you will save people a lot of time debugging travis builds, because the feedback cycle is very long... (modify config -> seeing result on travis).

I have still been debugging travis build all day, because switching to bionic breaks wasm-pack...

@xd009642
Copy link
Owner

xd009642 commented Mar 2, 2020

hmm maybe try using cargo install to install tarpaulin then, or the prebuilt docker image might work better and be a quicker CI time for you?

Made some tweaks
@xd009642
Copy link
Owner

xd009642 commented Mar 2, 2020

Also I made some tweaks to the readme, let me know if these improve things

Copy link
Contributor Author

@najamelan najamelan left a comment

Choose a reason for hiding this comment

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

LGTM. One thing I wonder about is why cargo install is run before cache. Isn't before cache after running everything to make sure something is in the cache for the next run? Or more usual, deleting something to prevent it from being cached. I wonder if this succeeds on a very first run.

@xd009642
Copy link
Owner

xd009642 commented Mar 2, 2020

Ah that's likely a hangover from when tarapulin was tied to the specific version of nightly that built it so caching meant a segfault whenever the nightly updated. Can remove it for crates that aren't relying on nightly features that may cause things to go wrong. I'll just do that now

change before_cache to before_script
@xd009642
Copy link
Owner

xd009642 commented Mar 3, 2020

Okay I'm gonna merge this now

@xd009642 xd009642 merged commit a23f40f into xd009642:develop Mar 3, 2020
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.

2 participants