Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

C 0.09 (new ​​formula) #37403

Closed
wants to merge 9 commits into from
Closed

C 0.09 (new ​​formula) #37403

wants to merge 9 commits into from

Conversation

janten
Copy link

@janten janten commented Mar 5, 2015

C is a shell script that compiles and executes C source code in the shell.

C is a shell script that compiles and executes C source code in the shell.
@janten janten changed the title Add Formula for the "c" script C 0.06 Mar 5, 2015
@janten janten mentioned this pull request Mar 5, 2015
@janten janten changed the title C 0.06 C 0.06 (new ​​formula) Mar 5, 2015
end

test do
system "c", "--help"
Copy link
Member

Choose a reason for hiding this comment

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

Can this test be modified to do something more substantial than e.g. --version or --help? See cmake.rb for an example of an application formula with a good test and tinyxml2.rb for an example of a library formula with a good test. Thanks!

Choose a reason for hiding this comment

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

@MikeMcQuaid Hey, currently we have unit testing. Can we just call that?

test do
    Dir.chdir("tests") do
        system "test.sh"
    end
end

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 be good if you can actually just use an invocation of the program to e.g. run a small script. See cmake.rb for something similar.

@MikeMcQuaid
Copy link
Member

Note to other @Homebrew/owners: this is a 10 day old but pretty popular project.

@janten janten changed the title C 0.06 (new ​​formula) C 0.07 (new ​​formula) Mar 5, 2015
@bruno-
Copy link
Contributor

bruno- commented Mar 5, 2015

I'd very much love to see this accepted/merged.

@ryanmjacobs
Copy link

Okay, v0.08 removes the gnu-sed dependency. Can we update c.rb to use that? Thanks.

@LinusU
Copy link
Contributor

LinusU commented Mar 6, 2015

@janten As soon as @ryanmjacobs tags v0.09 (which doesn't use sed at all) we should update the formula to use that and remove the dependency on gnu_sed.

Great work!

@ryanmjacobs
Copy link

@LinusU Okay, I've pushed v0.09. 😀

@LinusU
Copy link
Contributor

LinusU commented Mar 6, 2015

Nice!

@janten Here are the new values:

  url "https://github.com/ryanmjacobs/c/archive/v0.09.tar.gz"
  sha1 "41d284be0f9d59d45b9cc14e98dbe01ef060c1c1"

@janten
Copy link
Author

janten commented Mar 6, 2015

I just downloaded v0.09 myself and get the SHA1 sum as a17ad8071d68cb8ce9b3e0f11cff8ff9701f2792. I have updated the formula accordingly.

@LinusU
Copy link
Contributor

LinusU commented Mar 6, 2015

Hmm, that's strange. Well, as long as it works.

Here is how I got it:

$ curl -s https://github.com/ryanmjacobs/c/archive/v0.09.tar.gz | shasum
41d284be0f9d59d45b9cc14e98dbe01ef060c1c1  -

@janten
Copy link
Author

janten commented Mar 6, 2015

Looks like SHA1 is now deprecated in favor of SHA256, so I updated the formula again.

end

test do
system "echo", "int main(void){return 0;}", "|c"
Copy link
Member

Choose a reason for hiding this comment

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

(testpath/"test.c").write "int main(void){return 0;}"
system bin/"c", "test.c"

Copy link
Member

Choose a reason for hiding this comment

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

@janten Remind you a bit, current test case is incorrect.

Copy link
Author

Choose a reason for hiding this comment

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

No, it isn't. c takes input from stdin just fine and the test is meant to be run after the installation, when c will already be in $PATH, not in bin/

Copy link
Member

Choose a reason for hiding this comment

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

First of all, system "echo", "int main(void){return 0;}", "|c" equals echo "int main(void){return 0;}" "|c" in shell. The pipe is fake.

Second, all test have to use full path. Users' environment setting is irrelevant.

Copy link
Author

Choose a reason for hiding this comment

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

I'll happily admit that I do not know anything about ruby (and seeing this, I don't really want to deal with it) – my guess was that I would need to write system "echo", … "\"|c\"" to get a string instead of a pipe. I have updated the test accordingly. Even though, I fail to see any reason to use a source code file instead of a simple echo and from other formula's tests I gather that the path should be read from a variable as #{bin} instead of being literal bin/.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to read from pipe. Make test like this:

pipe_output(bin/"c", "int main(void){return 0;}")

As for your question on #{bin} and bin/, they are both OK. "#{bin}/c" equals (bin/"c").to_s.

@MikeMcQuaid
Copy link
Member

The last thing I'm a bit hesistant about with this formula: this is not the thing people think of when they think of c. Perhaps the formula name should be something slightly longer to avoid confusion?

@janten
Copy link
Author

janten commented Mar 7, 2015

I can not really think of any other name for it and I guess there is not much of a problem since one cannot install C, the language, anyway. IMHO the x in brew install x should also match the actual command name after the installation. I have repeatedly tried to install vowpal wabbit with brew install vw in the past, where the correct command would have been brew install vowpal-wabbit, which is not really consistent with the installation command for gcc being brew install gcc and not brew install gnu-compiler-collection.

In the end the author should decide the name of his program and as long as this tool is called "c", the formula should have the same name.

@tdsmith
Copy link
Contributor

tdsmith commented Mar 7, 2015

I have the same concern; it's difficult to imagine an unambiguous and discoverable name for a project called c. Maybe a tap is the best solution.

@ryanmjacobs
Copy link

I agree with @janten. It seems intuitive that brew install c would install the c command. No one is going to think that brew install c would install the C language itself, as one cannot possibly do that. The project won't be too ambiguous because the formula does link to a homepage.

@MikeMcQuaid
Copy link
Member

No one is going to think that brew install c would install the C language itself, as one cannot possibly do that.

Except someone who doesn't have a huge amount of familiarity with C and doesn't know that it isn't like e.g. ruby.

@MikeMcQuaid
Copy link
Member

Please do what @xu-cheng has asked; either write to a file or use pipe_output.

@janten janten changed the title C 0.07 (new ​​formula) C 0.09 (new ​​formula) Mar 8, 2015
@ryanmjacobs
Copy link

Any ideas why the tests are failing?

@tdsmith
Copy link
Contributor

tdsmith commented Mar 9, 2015

Looks like flakiness on our end; we're having some trouble with our CI hardware.

@jacknagel
Copy link
Contributor

I am passing on this due to the naming issue. In my opinion, "c" is far too ambiguous. Other maintainers, feel free to overrule me, but this has been sitting here untouched for two months.

@jacknagel jacknagel closed this May 9, 2015
@MikeMcQuaid
Copy link
Member

I agree.

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants