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

Allow builds to fail #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow builds to fail #4

wants to merge 1 commit into from

Conversation

0xADD1E
Copy link

@0xADD1E 0xADD1E commented Aug 31, 2019

In some cases, a build would fail (due to a particular malformed scheme)
but we still want to be able to build the correctly formed schemes.
To this end, this separates building a file into a fn -> Result.
If one of the schemes fails, we log it and move on.

Additionally (to make it more clear if a scheme fails to build,
some of the visual noise (messages for each created file) have been
taken down to debug level log messages.

In some cases, a build would fail (due to a particular malformed scheme)
but we still want to be able to build the correctly formed schemes.
To this end, this separates building a file into a `fn -> Result`.
If one of the schemes fails, we log it and move on.

Additionally (to make it more clear if a scheme fails to build,
some of the visual noise (messages for each created file) have been
taken down to debug level log messages.
@ilpianista
Copy link
Owner

Hi,
what base16-builder-php does when a scheme fails to build? I'd like to avoid to add a behaviour which isn't included in the builder specs.

May you please try to get this behaviour accepted in the specs, first?

@0xADD1E
Copy link
Author

0xADD1E commented Sep 3, 2019

So, to address the two points...

The specification

The specs are (deliberately?) fairly vague with regard to the incredibly low-level details of builders. I don’t think error handling would disagree with the letter of the specification, and almost certainly not the spirit.

Comparison to builder-php

Builder-PHP has some different behaviour than this builder in the first place. The main one I noticed was the fact that builder-php shells out to git (which tracks branches nicer than libgit2), instead of doing the libgit2 checkout. This was generally the first reason I added the error handling, was that on subsequent updates, the branch to fetch was hard coded as origin/master, which in the case of the horizon scheme, doesn’t exist. For the rust builder this aborts, and is somewhat inconvenient.

I also haven’t gotten the opportunity to investigate the build failures themself, but it could be related to this. I’ll do more triage later today and write up a proper issue for the origin/master hard coding (particularly if I can figure out the libgit2 way of finding the upstream branch), and if it’s not related, the build failures

@peterkos
Copy link

This is cool! +1 that handling a not-working scheme we try to reference shouldn't crash the program.

I tried running against this branch and got the two following errors:

# target/debug/base16-builder update
thread 'main' panicked at src/main.rs:309:27:
Failed to clone: remote authentication required but no callback set; class=Http (34); code=Auth (-16)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

# target/debug/base16-builder
thread 'main' panicked at src/main.rs:321:27:
Failed to clone: remote authentication required but no callback set; class=Net (12)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Which seem to be lingering .unwraps() elsewhere, but then I got this one:

target/debug/base16-builder
thread 'main' panicked at src/main.rs:183:51:
called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

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