search

HenrikBengtsson / affxparser

🔬 R package: This is the Bioconductor devel version of the affxparser package.
http://bioconductor.org/packages/devel/bioc/html/affxparser.html
7 stars 3 forks source link

Breaks with new Windows gcc 3.9.3 toolchain #22

Closed kasperdanielhansen closed 8 years ago

kasperdanielhansen commented 8 years ago

Some information on bioc-devel. Speculating that it has to do with us (re)defining various windows macros in the code in /src. Link to current build report.

kasperdanielhansen commented 8 years ago

I'll try to get a VM running, but it'll at least be a few days, so it would be great if @HenrikBengtsson could take a look first.

HenrikBengtsson commented 8 years ago

I'll try to look at this on Sunday (the earliest), i.e. understand the problem etc.

dtenenba commented 8 years ago

Just to capture a bit about what has been tried already:

I asked Jim Hester about affxparser and he said:

affxparser - Looks like they are overriding the WINVAR constant (

https://github.com/Bioconductor-mirror/affxparser/blob/master/src/R_affx_constants.h#L17 ), which is causing the windows symbols to be missed. You may want to try commenting that out (plus it feels wrong to be hardcoding that to Windows XP anyway…)

However, I did try commenting out the WINVAR constant and that did not help.

@jimhester

HenrikBengtsson commented 8 years ago

So, today Peter D announces Extending the beta period for R 3.3.0 till April 25, Final release on May 3 due to:

Due to delays in implementing an updated Windows toolchain for CRAN, R Core has found it unsafe to go with our usual 1-week beta testing period. [...]

[...] Accordingly, if we have too little time to fix issues discovered in the beta period, we might have to back out and be stuck with the old tools for another year.

In other words, our issues might be unrelated to the affxparser src/ code. For this reason (and priorities), I would be will to wait and see. But see also below.

About WINVER; there are two instances of this in the package:

#ifdef _MSC_VER
15  #define _strtoui64 strtoull /* for TsvFile.cpp */
16  #include <w32api.h>
17  #define WINVER WindowsXP    /* for Util.cpp, via TsvFile.cpp */
18  #endif

This was committed by @mtmorgan on 2008-03-04.

239 #ifndef WINVER
240 #define WINVER 0x0502
241 #endif

This was committed by me on 2012-03-05. For the latter I made the following NEWS comment:

Version: 1.27.4 [2012-03-05]
o BUG FIX: affxparser would not build on Windows with the new
  Rtools toolchain (Rtools 2.15.0.1915-1919).

So, it's not unlikely we have to fix things in affxparser to get it to build (also) with the new gcc 4.9.3 toolchain. On the other hand, it looks like there will be more changes made to that toolchain, so I prefer to wait a bit before digging into this.

dtenenba commented 8 years ago

Hi Henrik,

I don't believe that there will be changes to the toolchain. Certainly not the compiler versions used. I think the delay is so that we can find and fix issues in packages that do not install under the new toolchain. Therefore IMO that's what we should use this time for. I don't think waiting will serve any purpose. Let me know if there is anything I can do to help. Thanks, Dan

dtenenba commented 8 years ago

Tagging @jimhester in case he has more thoughts on this.

kasperdanielhansen commented 8 years ago

@HenrikBengtsson Any progress on this? I have managed to get a VM with Windows 7 up and running, (but I still need to get R and Emacs working).

jimhester commented 8 years ago

This should be fixed by https://github.com/HenrikBengtsson/affxparser/pull/24. Essentially the embedded _mingw.h file needed to be updated (with a small patch).

dtenenba commented 8 years ago

Thanks @jimhester ! I can confirm that the package installs, builds, and checks with this change under the new toolchain.

Can whoever merges this please add a version bump in DESCRIPTION so that the fixed package propagates. Otherwise it will make our build system cleaner but not help end users.

Thanks!

HenrikBengtsson commented 8 years ago

Thanks so much for all this; I'm not sure if I'd be able to make those changes myself. I've merged this into master branch.

Good to hear it works out on the Bioc server(s). I was a bit concerned about backward compatibility, but it seems to work for older versions too. At least according to https://ci.appveyor.com/project/HenrikBengtsson/affxparser/build/1.0.69 and https://travis-ci.org/HenrikBengtsson/affxparser/jobs/120687377 (which both use gcc 4.6.3).

I'll try to push to Bioc SVN later tonight.

Also, I'll try to run a few more checks on old R versions.

I'm also trying to set up the new gcc 4.9.3 tool chain on my Windows machine now when there is a pre-build R 3.3.0 beta binary. It also seems to require some extra tweaks compared to previous generations of R/Rtools, but the following seem useful https://github.com/rwinlib/r-base/wiki/Testing-Packages-with-Experimental-R-Devel-Build-for-Windows.

@kasperdanielhansen, I'm using GNU Emacs 24.3.1 (i386-mingw-nt6.1.7601) on Windows 7.

dtenenba commented 8 years ago

@HenrikBengtsson Thanks. The beta binary was available before, but the link wasn't public until now for some reason. It will work with both toolchains, but you have to tweak it to recognize the toolchain you want to use.

You may find this helpful as well if I haven't already passed it on: https://github.com/Bioconductor/Bioconductor/blob/master/documentation/new-toolchain-setup.md

dtenenba commented 8 years ago

As for backward compatibility, the package should be used only with Bioc 3.3 and R-3.3 (and if users use biocLite() as they should, that is what will happen). Using this version of affxparser with an older R is not supported in any case. You could add Depends: R (>= 3.3) to DESCRIPTION to enforce this.

The appveyor config needs to be updated to use the new toolchain.

HenrikBengtsson commented 8 years ago

I've updated so that the package builds on both the old and the new tool chain, cf. 79752a0.

@dtenenba, there are various reasons why I want affxparser to be backward compatible, except from the fact that there are now Windows R 3.3.0 binaries out there that are using either gcc 4.6.3 or gcc 4.9.3. Although I agree that in the Bioconductor-verse users shouldn't mix-and-match across Bioc releases, there are also users out there that uses (for instance) affxparser outside of Bioconductor.

HenrikBengtsson commented 8 years ago

I'm trying to submit to Bioc SVN, but I get:

[HB-X201]{hb}: git svn dcommit --add-author-from
Committing to https://hedgehog.fhcrc.org/bioconductor/trunk/madman/Rpacks/affxparser ...
No changes
c316b3c58d3b7957bb3740e0ec4e1d362ce09bca~1 == c316b3c58d3b7957bb3740e0ec4e1d362ce09bca
Unable to extract revision information  from commit 95395eda00d5aa793f8f99682217c59118138144~1

I definitely have changes in my local branch when compared to the SVN server:

Merge: c316b3c d1b0f06
Author: hb <hb@aroma-project.org>
Date:   Tue Apr 5 15:23:13 2016 -0700

    Merge branch 'develop' into bioc/devel

    Conflicts:
        .make/Makefile
        DESCRIPTION
        NEWS
        README.md
        appveyor.yml
        src/_mingw.h

commit d1b0f0620dccf46f66b412a2e8d51ad72401534b
Author: hb <hb@aroma-project.org>
Date:   Tue Apr 5 15:19:07 2016 -0700

    Undo - not needed

commit c316b3c58d3b7957bb3740e0ec4e1d362ce09bca
Merge: 65d29b2 2e4bf57
Author: hb <hb@aroma-project.org>
Date:   Tue Apr 5 15:17:34 2016 -0700

    Merge branch 'master' of https://github.com/Bioconductor-mirror/affxparser i
nto bioc/devel

commit 79752a08f16b6f23464eb12440d1c4e89da39662
Author: hb <hb@aroma-project.org>
Date:   Tue Apr 5 15:06:19 2016 -0700

    WINDOWS: Package compiles on gcc-4.6.3 and gcc-4.9.3 toolchains [#22]
[...]

I have no idea what's wrong.

jimhester commented 8 years ago

git svn really doesn't deal well with merge commits, they really need to be fast-forward merges only to work.

You may need to manually cherry-pick the changes to the devel branch to get this to work, or just patch the diff and use SVN directly if that is easier.

dtenenba commented 8 years ago

@HenrikBengtsson let me and/or @jimhester know if we can help with this. Would love to see this in svn as soon as it can be figured out. Thanks.

HenrikBengtsson commented 8 years ago

Thanks for offering help.

What's holding me back now (in addition trying to understand Git better) is what the model should look like when the Git-SVN connection is more or less doomed to be broken without major hassles going forward. Basically, it seems like Git will be the main repository with a clean/full commit history and Bioc SVN will be synced to just for the purpose of building and releasing via Bioc. Modulo small regular version bumps, all updates should from now on be done to Git. (Ideally there should be a "read-only" note for the SVN repos on the package page). It seems like that decision will have to be done now before committing.

So, I guess it's ok to do the poor man's solution and create an SVN patch and do a single commit to Bioc. I'm going to be away all day and fairly busy tomorrow too, so feel free to do the patch (version needs to be bumped) and submit to Bioc SVN.

Thxs

dtenenba commented 8 years ago

Yes, it's a mess. Some of it is my fault because of the way the old git-svn bridge worked. Some of it is just the nature of the beast, with svn not being able to deal with non-linear commit history in git. I apologize for the pain.

Another way of looking at this is that all development should be done on svn (I know how horrible that sounds). But none of this would be an issue if we stuck to svn.

There are vague rumors that Bioconductor will switch its canonical version control system to git at some point but you did not hear it from me and I certainly don't know when or if....

Anyway, having said all that, I see that someone (Kasper?) made the relevant commits to svn and I am able to install that package on the new toolchain. It also passes check.

(Incidentally affxparser has no vignette. )

So from my point of view the issue can be closed. Thanks everyone!

kasperdanielhansen commented 8 years ago

Ok, I think I fixed this. But I might have messed up the commit history in SVN. At least the code now reflects what is in Github.

Following the instructions from the howto on git mirrors, I did

git merge master --log --no-ff

But now I get

svn log DESCRIPTION
r115964 | khansen | 2016-04-07 14:09:40 -0400 (Thu, 07 Apr 2016) | 1 line

version bump
------------------------------------------------------------------------
r115963 | khansen | 2016-04-07 13:59:38 -0400 (Thu, 07 Apr 2016) | 26 lines

Merge branch 'master' into devel

* master: (512 commits)
  Undo - not needed
  WINDOWS: Package compiles on gcc-4.6.3 and gcc-4.9.3 toolchains [#22]
  ROBUSTNESS: make now asserts BINPREF is set on R (>= 3.3.0) for Windows
  Update Date and Authors
  Update _mingw.h for the 4.9.3 toolchain
  CLEANUP: rep(x, length=n) -> rep(x, times=n)
  CLEANUP: rep(x, length=n) -> rep(x, times=n)
  Appveyor CI: Save error logs as artifacts
  Updated pkg dot files
  Updated pkg dot files
  Bump pkg dependencies
  fixed a bug related to R.h and extern C reproted by Brian Ripley
  fixed a bug related to R.h and extern C reproted by Brian Ripley
  Bump develop version
  And the Date
  Hmm... Bioc devel should be 1.43.0 (not 1.42.0)
  Update NEWS to reflect BioC devel (3.3)
  Woops; wrong version
  Update NEWS for affxparser 1.42.0 (BioC 3.2 relelease)
  Commit made by the Bioconductor Git-SVN bridge.
  ...
kasperdanielhansen commented 8 years ago

Also note that in SVN I did a version bump to 1.43.2

HenrikBengtsson commented 8 years ago

Thanks all. @dtenenba, absolutely not worries about the Git-SVN "mess". It seems to be independent on Bioc. It's complicated to keep Git and SVN in sync in general, especially when less-Git-savvy people like me try to do it (certain things are a bit of trial and error for me).

I'll close this issue, but feel free to keep give feedback if you'd like to.