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

liquid-dsp 1.2.0 (new formula) #2664

Closed
wants to merge 6 commits into from

Conversation

muellermartin
Copy link
Contributor

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --strict --online <formula> (after doing brew install <formula>)?

@dunn
Copy link
Contributor

dunn commented Jul 3, 2016

@dunn dunn added the new formula PR adds a new formula to Homebrew/homebrew-core label Jul 3, 2016
@muellermartin
Copy link
Contributor Author

Yup, already saw that, but it seems to be a weird inline assembler (?) related error and I'm curious why this only only happens on Mavericks. I guess this could be a compiler issue...

@dunn
Copy link
Contributor

dunn commented Jul 3, 2016

Could you report it to the developers? Thanks!

The test build on OS X Mavericks failed with errors like "no such
instruction: `vmovss LC1(%rip), %xmm2'" which might relate to AVX
intrinsics - at least that's what I've found in a similar issue:
OP2/PyOP2#471
The suggested fix is to add the compiler flag "-Wa,-q" which this
commit tries to do.
@muellermartin
Copy link
Contributor Author

@BrewTestBot Please test this again.

@MikeMcQuaid
Copy link
Member

@muellermartin Why did you close/reopen this?

@muellermartin
Copy link
Contributor Author

muellermartin commented Jul 4, 2016

@MikeMcQuaid Sorry, I accidentally clicked the wrong button :(

@muellermartin
Copy link
Contributor Author

I just wanted to note that I found a similar issue with OP2/PyOP2#471 which points out that this could be related to AVX instructions. With the last commit I want to check if the build error on Mavericks can be resolved with the suggested fix (adding -Wa,-q to the CFLAGS) which is also suggested in this StackOverflow thread.

@muellermartin
Copy link
Contributor Author

muellermartin commented Jul 4, 2016

Build looks good, no errors - even on Mavericks. I'll report that issue upstream. Until then this package could be used.

Update: This issue is now reported upstream: jgaeddert/liquid-dsp#48

depends_on "fftw" => :recommended
# libfec could be an optional dependency, but it's not available, yet

fails_with :clang do
Copy link
Contributor

Choose a reason for hiding this comment

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

We've been extremely 👎 on accepting new formulae which won't build against Clang, particularly if there's no upstream work in progress on making it Clang compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's understandable. Maybe I can get it to work under LLVM/Clang by fiddling around...

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've found a way to successfully build it with LLVM/Clang instead of GCC with a simple patch which already has an upstream issue (see jgaeddert/liquid-dsp#50). Should I submit another pull request or add it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Submitting a PR & then using that PR in patch do format if it applies cleanly here would be terrific.

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've already submitted a patch upstream (jgaeddert/liquid-dsp#51) Do I understand it correctly that I can add it here in this PR? Is an embedded patch (patch :DATA + __END__) O.K.?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it will apply cleanly, we'd prefer the:

patch do
  url "https://github.com/jgaeddert/liquid-dsp/pull/51.patch"
  sha256 ""
end

Format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, didn't know you also can get a "raw" patch file out of a PR hosted on GitHub. Neat and thanks for this tip!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added commit with patch.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aye, you can get diffs if preferred by simply switching it to diff as well. Very handy feature.

def install
system "./reconf"
system "./configure", "--prefix=#{prefix}"
# Note: "make install" in one step does fail
Copy link
Member

Choose a reason for hiding this comment

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

Could you report this issue upstream? Thanks!

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 do that and report back as soon as it's done!

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've now reported this upstream as jgaeddert/liquid-dsp#53 and included a simple PR. Should I add a "patch" with inreplace in the formula to change the relevant file in the Makefile (makefile.in) or just add/replace the comment to mention the upstream bug?

Copy link
Member

Choose a reason for hiding this comment

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

Just add the comment, thanks 👍

@muellermartin
Copy link
Contributor Author

I've now added a comment on another upstream issue as suggested. The reported upstream issues (jgaeddert/liquid-dsp#48 and jgaeddert/liquid-dsp#52) do have patches/workarounds until they get resolved and released which seems to take time due to stalling maintenance, but I've heard that this is beeing worked on.
If you guys are satisfied with the current state of the formula, feel free to merge it.

@dunn
Copy link
Contributor

dunn commented Jul 26, 2016

I might wait another day or two for upstream to reply to your tickets (thanks for filing them!)

@MikeMcQuaid
Copy link
Member

Closing until we get a new stable release with these patches included.

@MikeMcQuaid MikeMcQuaid closed this Sep 3, 2016
muellermartin added a commit to muellermartin/homebrew-core that referenced this pull request Mar 26, 2017
@ralfbiedert ralfbiedert mentioned this pull request Sep 20, 2017
4 tasks
@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 upstream issue An upstream issue report is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants