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

libtecla: test added #11844

Merged
merged 1 commit into from
Mar 30, 2017
Merged

libtecla: test added #11844

merged 1 commit into from
Mar 30, 2017

Conversation

lordqwerty
Copy link
Contributor

  • 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>)?

@bfontaine bfontaine changed the title Change build to deparallelize and test added libtecla: Change build to deparallelize and test added Mar 30, 2017
Copy link
Member

@fxcoudert fxcoudert left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! The test is a very good idea.

ENV.deparallelize
system "./configure", "--prefix=#{prefix}", "--mandir=#{man}"
system "make", "install"
ENV.deparallelize do
Copy link
Member

Choose a reason for hiding this comment

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

ENV.deparallelize was already present, and making it into a block is a no-op.

system "./configure", "--prefix=#{prefix}", "--mandir=#{man}"
system "make", "install"
ENV.deparallelize do
system "./configure", "prefix=#{prefix}", "mandir=#{man}"
Copy link
Member

Choose a reason for hiding this comment

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

This is a traditional configure, the options need to be of the form --prefix


test do
(testpath/"test.c").write <<-EOS.undent
#include <stdio.h>
Copy link
Member

Choose a reason for hiding this comment

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

stdio.h and string.h should not be necessary

@fxcoudert
Copy link
Member

Thanks, can you squash the commits into one, with commit message libtecla: add test

if(!gl)
return 1;

gl = del_GetLine(gl);
Copy link
Member

Choose a reason for hiding this comment

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

what value is gl assigned to in case of a failure? can you add something similar to lines 35-36 above?

Copy link
Member

Choose a reason for hiding this comment

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

It always returns NULL, says the doc.

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 don't think this matters. 35 - 36 cover failure enough for test. Line 38 deleting as not doing anything of significance.

Copy link
Member

Choose a reason for hiding this comment

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

then just don't assign this to gl

Copy link
Member

Choose a reason for hiding this comment

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

It's fine…

@lordqwerty
Copy link
Contributor Author

I've done something horribly wrong... gulp

@fxcoudert
Copy link
Member

You have picked up an unrelated commit.

@lordqwerty
Copy link
Contributor Author

Sorry for the trouble.

@maxim-belkin
Copy link
Member

How about making this a tad more compact?

  test do
    (testpath/"test.c").write <<-EOS.undent
      #include <locale.h>
      #include <libtecla.h>

      int main(int argc, char *argv[]) {
        GetLine *gl;
        setlocale(LC_CTYPE, "");
        gl = new_GetLine(1024, 2048);
        if (!gl) return 1;
        return 0;
      }
    EOS

    system ENV.cc, "test.c", "-L#{lib}", "-ltecla", "-o", "test"
    system "./test"
  end

@lordqwerty
Copy link
Contributor Author

Please let me know if you would like further changes. If not I believe ready to merge.

@maxim-belkin
Copy link
Member

I think it is worth combining lines 28 and 30

@fxcoudert fxcoudert merged commit 16565cc into Homebrew:master Mar 30, 2017
@fxcoudert
Copy link
Member

Thanks @lordqwerty for your contribution!

@lordqwerty lordqwerty changed the title libtecla: Change build to deparallelize and test added libtecla: test added Mar 30, 2017
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants