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

Salmon gcc7 #10344

Merged
merged 48 commits into from
Aug 30, 2018
Merged

Salmon gcc7 #10344

merged 48 commits into from
Aug 30, 2018

Conversation

rob-p
Copy link
Contributor

@rob-p rob-p commented Aug 9, 2018

  • I have read the guidelines for bioconda recipes.
  • This PR adds a new recipe.
  • AFAIK, this recipe is directly relevant to the biological sciences (otherwise, please submit to the more general purpose conda-forge channel).
  • This PR updates an existing recipe.
  • This PR does something else (explain below).

This is a test to see if we can build salmon using the new compiler (gcc7) ... at least on linux. This works locally using the "bootstrap" method, so let's see what happens on Circle CI.

@rob-p
Copy link
Contributor Author

rob-p commented Aug 9, 2018

Hrmm, this seemed to work .... not so sure what's different between this and before when it failed. Going to try uncommenting OSX now too.

Try building on OSX
@rob-p
Copy link
Contributor Author

rob-p commented Aug 9, 2018

Hrmmm .. looks like this icu58 issue is still popping up on OSX, but has gone away under linux on the newer compiler. I'd really like to figure this out so I can provide a mac executable through bioconda.

@@ -0,0 +1,6 @@
c_compiler:
- gcc # [linux]
- clang # [osx]
Copy link
Member

Choose a reason for hiding this comment

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

Rob can you try to replace clang here with GCC as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok; just pushed. I didn't even know that building with GCC on OSX was an option here :).

trying GCC/GXX even on Mac
@rob-p
Copy link
Contributor Author

rob-p commented Aug 9, 2018

It used clang anyway, it seems ....

Copy link
Contributor Author

@rob-p rob-p left a comment

Choose a reason for hiding this comment

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

Ahhh, so it wasnt even fixed on linux yet. Well, at least the error seems the same in both cases! Is this affecting any other packages?

@bgruening
Copy link
Member

Not really as we do not build many pkgs with the new compilers. My assumption is that we need to rebuild iuc with the new compilers before we build Salmon :(

@rob-p
Copy link
Contributor Author

rob-p commented Aug 9, 2018

Can that be done here, or is that coming from conda-forge? Can we submit PRs there?

@bgruening
Copy link
Member

Its a larger effort we are working on. The migration to the new compilers should happen soon. But it will take time, nothing we can influence right now. Its mainly CI build time dependent ... If someone want to buy more CI build-time - more then welcome :)

I will keep you posted, sorry.

@rob-p
Copy link
Contributor Author

rob-p commented Aug 9, 2018

Ok, so I get it to build locally if I force the boost from anaconda cloud and not the one from conda-forge. The conda-forge ppl aren't building a c++11 enabled boost :(. Perhaps there is a more elegant way to do this.

revert to clang on OSX.
@rob-p
Copy link
Contributor Author

rob-p commented Aug 9, 2018

ahh, so no we are getting something different popping up on OSX. Perhaps this one is solvable. Any thoughts @bioconda/osx ?

Rob Patro and others added 5 commits August 9, 2018 14:13
@rob-p rob-p mentioned this pull request Aug 10, 2018
5 tasks
@acaprez
Copy link
Contributor

acaprez commented Aug 10, 2018

librt is part of glibc, so yeah, you'd need to remove that flag entirely. Hopefully you won't get an unresolved symbol error then...

@rob-p
Copy link
Contributor Author

rob-p commented Aug 28, 2018

Now it's back at a damn icu58 linker issue under OSX ... . But, the icu being pulled in is v58. From the build log:

   icu:             58.2-hfc679d8_0               conda-forge

why won't it link?

There are also some even stranger errors:

 NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
  "vtable for std::__1::basic_istream<char, std::__1::char_traits<char> >", referenced from:
      boost::cpp_regex_traits<char>::toi(char const*&, char const*, int) const in libboost_regex.a(instances.o)

and

  "std::__1::cout", referenced from:

etc. Perhaps this boost was not linked against libstdc++, given that it's on OSX?

ok, let's go all in on the new stuff and try to fix it from that direction
try different boost on osx?
this is a frustrated commit message ...
Try boost instead of libboost --- do we get the same error on OSX?
See if we can use boost instead of libboost on linux
nope ... `boost` instead of `libboost` fails under linux.
horrible, horrible, horrible ...
nope.  nope, nope, nope --- we *need* `libboost` on linux for now.  Why doesn't it work on OSX?
update md5sum
50th time's a charm?
51st time's a charm ... I'm gonna guess not.
@rob-p
Copy link
Contributor Author

rob-p commented Aug 29, 2018

Holy shit!

Ok --- last attempt *worked*!  Now, try to make everything C++14, and re-enable linux build.
have to update md5sum
@rob-p
Copy link
Contributor Author

rob-p commented Aug 29, 2018

Note : This is currently pulling from develop. I will be cutting a new release (tagged) imminently. Please don't merge this yet.

try and build tagged v0.11.3
@rob-p rob-p merged commit c1a7153 into bioconda:master Aug 30, 2018
Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Holy cow! Amazing @rob-p. Thanks a lot for all the effort - very appreciated.

host:
- boost
- icu
- libboost # [linux]
Copy link
Member

Choose a reason for hiding this comment

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

the selector is now not needed here ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @bgruening --- yes, I agree that this is not needed (and probably the same with the libcxx below). Should I try to bump the version and remove these and see if it compiles?

Copy link
Member

Choose a reason for hiding this comment

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

That would be great, yes. Thanks.

- boost
- icu
- libcxx
- libboost # [linux]
Copy link
Member

Choose a reason for hiding this comment

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

same here

- {{ compiler('cxx') }}
- {{ compiler('c') }}
#- clangdev =={{ llvm_version }} # [osx]
#- libcxx =={{ llvm_version }} # [osx]
- libcxx
Copy link
Member

Choose a reason for hiding this comment

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

is this needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants