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

lm4tools 0.1.3 (new formula) #25140

Closed
wants to merge 1 commit into from
Closed

lm4tools 0.1.3 (new formula) #25140

wants to merge 1 commit into from

Conversation

dancek
Copy link
Contributor

@dancek dancek commented Mar 12, 2018

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Lm4tools is used for flashing and debugging TI ARM Cortex-M microcontrollers. I needed it and couldn't find a package so here's one.

depends_on "libusb"

def install
cd "lm4flash"
Copy link
Member

Choose a reason for hiding this comment

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

There is a top-level makefile, so we could simply run:

    system "make"
    bin.install "lm4flash/lm4flash"
    bin.install "lmicdiusb/lmicdi"

Also, could you open an issue with upstream asking that they add an install target to their makefile? That way we could do make install PREFIX=...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no top-level makefile in v0.1.2 (the latest release). :( It's a later addition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upstream issue for the install target: utzig/lm4tools#27

Copy link

@utzig utzig Mar 12, 2018

Choose a reason for hiding this comment

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

I added a PR which adds the functionality (utzig/lm4tools#28), I tested on Linux only, would be glad if it looks OK and you can check with Homebrew before I merge. Btw, I could make a release 0.1.3 (or maybe 0.1.4) afterwards...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change this once 0.1.3 or 0.1.4 is released. I'm fine with keeping this PR waiting until then.

end

test do
system "#{bin}/lm4flash", "-V"
Copy link
Member

Choose a reason for hiding this comment

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

We are going to need a test that does more than that…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The software needs connected hardware to do anything useful. I can write a proper test, but that would break CI.

Copy link

Choose a reason for hiding this comment

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

Could also grep for the version, or even try flashing any file which should output "Unable to find any ICDI devices" when no device is connected (like in a CI). Of course, this would fail if run locally with an actual device connected...

Copy link
Member

Choose a reason for hiding this comment

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

try flashing any file which should output "Unable to find any ICDI devices" when no device is connected

This is good. Or flashing something that is an invalid file and check that it says so (if it does).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented the suggested test. It will of course fail if a device is connected, but that is probably obvious if anyone runs the test and sees that "Unable to find any ICDI devices" didn't match the stderr.

@fxcoudert fxcoudert added the new formula PR adds a new formula to Homebrew/homebrew-core label Mar 12, 2018
end

test do
require "open3"
Copy link
Member

@fxcoudert fxcoudert Mar 12, 2018

Choose a reason for hiding this comment

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

Let's simply do:

output = shell_output("#{bin}/lm4flash - 2>&2")
assert_match "Unable to find any ICDI devices", output

@dancek dancek force-pushed the lm4tools branch 2 times, most recently from aba3e07 to c6f1ad7 Compare March 13, 2018 09:57
@dancek dancek changed the title lm4tools 0.1.2 (new formula) lm4tools 0.1.3 (new formula) Mar 13, 2018
@dancek
Copy link
Contributor Author

dancek commented Mar 15, 2018

This is ready for merge. Upstream made a new release and I've implemented the suggestions that came up in the review.

@fxcoudert fxcoudert closed this in f5b0b8f Mar 16, 2018
@fxcoudert
Copy link
Member

Merged (with the HEAD removed, as we don't enable head builds for most formulas… let's wait and see how often this is used).

Thanks @dancek for the pull request!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants