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
shogun: import from homebrew/science #23479
Conversation
@vigsterkr it would have been preferable to reopen the first PR and force push rather than opening and closing three separate PRs. Please do not close this one if you intend to reopen another one. |
@ilovezfs tried to reopen the pr but it didn't allow after closing & force pushing... anyhow i ran out of ideas how to fix this issue... so i guess with time this will get autoclosed by the stale bot |
for the record the first was closed by stale bot that i could not reopen again hence the second pr |
@vigsterkr ah OK. Yeah I could have reopened the one stale bot closed for you if you asked. As for force pushing not letting you reopen something, all you have to do is force push the last commit shown in the PR in the GUI, and then it will let you reopen. Then you can force push whatever you want over that. |
@ilovezfs ok so as you can see now all 3 versions of osx successfully builds and runs the tests, only due to the following errors the jobs fail:
i dont know how to silence 1 & 2, i can add to the patch the fix for the 3rd error |
You can't silence them but they will go away once it's merged because test-bot will start using |
@ilovezfs ok do you want me to change anything in this patch your its good for merge? |
that needs to be fixed |
@ilovezfs ok i'll add the fix for it to the patch |
Formula/shogun.rb
Outdated
"-DBUILD_META_EXAMPLES=OFF", | ||
"-DINTERFACE_PYTHON=ON", | ||
"-DINTERFACE_JAVA=ON", | ||
"-DJBLAS=#{libexec}/jblas-1.2.3.jar", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not hard-code this version number. You could do
"-DJBLAS=#{libexec}/jblas-#{resource("jblas").version}.jar",
Formula/shogun.rb
Outdated
end | ||
|
||
test do | ||
(testpath/"test.cpp").write <<-EOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use squiggly heredocs <<~EOS
Formula/shogun.rb
Outdated
int main(int argc, char** argv) | ||
{ | ||
init_shogun_with_defaults(); | ||
assert (std::strcmp(MAINVERSION, "6.1.3") == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not hard code this version.
assert (std::strcmp(MAINVERSION, "#{version}") == 0);
Formula/shogun.rb
Outdated
sha256 "dfbd03cba5e1a134a520d6c06aceaa3c5143cad638fc208150d980a44e5252cf" => :x86_64_linux | ||
end | ||
|
||
patch :DATA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use patch do
block(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've taken this from other formulas, as i want to have the patch part of the formula file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are legacy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i'm force to put the patches somewhere separately on the web?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there upstream commits that apply cleanly? Those can be used like
patch do
url "https://github.com/bitcoin/bitcoin/commit/1ec0c0a01c.patch?full_index=1"
sha256 "a1f761fe29f78e783cb4b55f8029900f94b45d1188cb81c80f73347ee2fdc025"
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did my share of complaining about it at one point. You're not the first to say that and you won't be the last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep i thought so, i'm just confirming the observation of others ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I like the fact that it keeps really long patches out of the main repository, which would distort contributor statistics whenever someone has a patch that is thousands of lines long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k i'm ready with the fixes... once the above mentioned pr gets merged i can push the fix for the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formula/shogun.rb
Outdated
url "http://shogun-toolbox.org/archives/shogun/releases/6.1/sources/shogun-6.1.3.tar.bz2" | ||
sha256 "57169dc8c05b216771c567b2ee2988f14488dd13f7d191ebc9d0703bead4c9e6" | ||
|
||
bottle do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just remove the entire bottle block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you mean the content not the, but keep
bottle do
end
or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete that too
Formula/shogun.rb
Outdated
sha256 "36ee86d5adbabc4fa2643a073f93d5504bdfed37a149a3a49f4dde259f35a750" | ||
end | ||
|
||
resource "jblas" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would put this above numpy
Formula/shogun.rb
Outdated
depends_on "xz" | ||
|
||
resource "numpy" do | ||
url "https://files.pythonhosted.org/packages/bf/2d/005e45738ab07a26e621c9c12dc97381f372e06678adf7dc3356a69b5960/numpy-1.13.3.zip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
current version is 1.14.0
Formula/shogun.rb
Outdated
|
||
mkdir "build" do | ||
system "cmake", "..", *args | ||
system "make" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it fail without the separate make step?
Formula/shogun.rb
Outdated
"-DLIB_INSTALL_DIR=#{lib}", | ||
] | ||
|
||
mkdir "build" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does an in-tree build fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Formula/shogun.rb
Outdated
EOS | ||
|
||
ENV.cxx11 | ||
cxx_with_flags = ENV.cxx.split + ["-I#{include}", "test.cpp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is overly fancy. I'd just do system ENV.cxx, "-std=c++11", ...
and remove the ENV.cxx11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there actually ENV.cxx14
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope and there won't be.
Formula/shogun.rb
Outdated
:PYTHONPATH => ENV["PYTHONPATH"], | ||
:LAPACKE_PATH => '#{Formula["lapack"].opt_lib}', | ||
} | ||
bin.env_script_all_files(libexec/"bin", env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this end up putting in bin
?
Formula/shogun.rb
Outdated
ENV["PYTHONPATH"] = libexec/"lib/python2.7/site-packages" | ||
ENV.prepend_create_path "PYTHONPATH", libexec/"vendor/lib/python2.7/site-packages" | ||
|
||
res = %w[numpy] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's just the one so the array and loop aren't needed. just resource("numpy").stage do
etc.
Formula/shogun.rb
Outdated
depends_on "eigen" | ||
depends_on "glpk" | ||
depends_on "hdf5" | ||
depends_on :java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed at run time if the user doesn't use the java feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should be
depends_on :java => ["1.7+", :build]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or actually it can probably be left out altogether since the jar is prebuilt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which jar? you need java to build the jar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i guess you want to do the same for python?
It's already the case since macOS comes with Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which jar? you need java to build the jar
I'm thinking of jblas
. I guess it builds more jars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it builds a jar itself, so jvm is required for build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. What is minimum Java needed? Probably
depends_on :java => ["1.7+", :build]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, we could go with 1.6 as well but it's EOL so better go with 1.7..... although that one is EOL as well :)
here it goes... |
Formula/shogun.rb
Outdated
url "http://shogun-toolbox.org/archives/shogun/releases/6.1/sources/shogun-6.1.3.tar.bz2" | ||
sha256 "57169dc8c05b216771c567b2ee2988f14488dd13f7d191ebc9d0703bead4c9e6" | ||
|
||
patch do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These belong under the dependencies but above the resources.
Formula/shogun.rb
Outdated
depends_on "eigen" | ||
depends_on "glpk" | ||
depends_on "hdf5" | ||
depends_on :java => ["1.7+", :build] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is build-time so it should be sorted alphabetically amongst the build time dependencies.
Formula/shogun.rb
Outdated
|
||
libexec.install resource("jblas") | ||
|
||
args = std_cmake_args + [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing is being optionally appended to this array so these should all be inlined below. See opencascade for an example.
So I'd add |
@ilovezfs in! |
So it looks like the |
Specifically, something is wrong with
if that |
i guess this would be enough...?
|
or should it be:
? |
That part seems to be failing despite not failing the whole build. Are you seeing that happen locally? |
mmm why is that setuptools called at all? |
The
Or is the only point of that part to get numpy installed before proceeding? |
it's only to trigger numpy having installed... |
OK so I think we just need
and the rest isn't doing anything. |
|
or should i just do |
It's not currently getting defined in ENV at all. The entire
is currently a no-op. If you need to set
(or should that be |
dropped all those things and added |
Formula/shogun.rb
Outdated
# https://github.com/shogun-toolbox/shogun/commit/fef8937d215db7 | ||
ENV.append_to_cflags "-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORES=0" | ||
|
||
ENV["PYTHONPATH"] = libexec/"lib/python2.7/site-packages" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this line needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well otherwise cmake will not find numpy...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't? Shouldn't the ENV.prepend_create_path "PYTHONPATH", libexec/"vendor/lib/python2.7/site-packages"
take care of that?
Formula/shogun.rb
Outdated
system "python", *Language::Python.setup_install_args(libexec/"vendor") | ||
end | ||
|
||
ENV["LAPACKE_PATH"] = Formula["lapack"].opt_lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not opt_prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noup i actually need /usr/local/..../lib
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
is showing
So I assume we should drop the readline dependency? And that the |
mmm that linkage is not going to be fulfilled for |
Should we scope |
and upgrade to 6.1.3
scoped, dropped, fixed.... |
Thanks for your first contribution to Homebrew, @vigsterkr! Without people like you submitting PRs we couldn't run this project. You rock! |
and upgrade to 6.1.3
with fix for vecLib