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

MR1: Remove special case for Cygwin for BLAS detection when installing fflas_ffpack #26353

Closed
sagetrac-gitlab-bot mannequin opened this issue Sep 27, 2018 · 11 comments
Closed

Comments

@sagetrac-gitlab-bot
Copy link
Mannequin

sagetrac-gitlab-bot mannequin commented Sep 27, 2018

<img src="https://secure.gravatar.com/avatar/c7d341e2b002754f2b21fbab3a3fd072?s=80&d=identicon" right, margin=5> Erik Bray (@embray) opened a merge request at https://gitlab.com/sagemath/sage/merge_requests/1:

We don't need this special case for Cygwin anymore. openblas and atlas packages should work without special-casing use of the netlib BLAS from Cygwin.

Component: porting: Cygwin

Author: Erik Bray

Branch/Commit: 09e289a

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/26353

@sagetrac-gitlab-bot sagetrac-gitlab-bot mannequin added this to the sage-8.4 milestone Sep 27, 2018
@embray

This comment has been minimized.

@embray
Copy link
Contributor

embray commented Sep 27, 2018

comment:3

First attempt at merge request synced from gitlab. A few bugs that happened (which for some reason didn't happen in testing):

  1. The post back to gitlab with the link to the ticket did not succeed. Somehow a typo I didn't have in testing crept in. I have already fixed that.

  2. For reasons I'm not sure, the ticket was created in an invalid state instead of "new". In fact, the ticket should probably created and immediately set to "needs_review".

The rest seemed to work OK. It will be good to have a mechanism to set other fields such as component and type, but for now I don't have a mechanism for that and instead assume it should be left up to project maintainers to set those fields.

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Sep 27, 2018

Changed commit from baa351b to 09e289a

@sagetrac-gitlab-bot
Copy link
Mannequin Author

sagetrac-gitlab-bot mannequin commented Sep 27, 2018

comment:4

New commits added to merge request. I updated the commit SHA-1. This was a forced push. New commits:

09e289awe don't need this special case for Cygwin anymore

@tscrim
Copy link
Collaborator

tscrim commented Sep 27, 2018

comment:5

You probably want a mechanism to set the real name in the authors field too as otherwise this ticket will get reverted back from a positive review. Should I do the review from the gitlab side or from the Sage side?

@embray
Copy link
Contributor

embray commented Sep 28, 2018

comment:6

Replying to @tscrim:

You probably want a mechanism to set the real name in the authors field too as otherwise this ticket will get reverted back from a positive review.

That's a good point. The names would have to match up though (or, I suppose, the release manager can always update the "trusted names" list.

Should I do the review from the gitlab side or from the Sage side?

Whichever you prefer.

@embray
Copy link
Contributor

embray commented Sep 28, 2018

Author: Erik Bray

@tscrim
Copy link
Collaborator

tscrim commented Sep 28, 2018

comment:8

LGTM.

@tscrim
Copy link
Collaborator

tscrim commented Sep 28, 2018

Reviewer: Travis Scrimshaw

@embray
Copy link
Contributor

embray commented Sep 28, 2018

comment:9

Just deployed an update to the bot which should address all the issues raised in this initial test ticket :)

@vbraun
Copy link
Member

vbraun commented Sep 29, 2018

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

No branches or pull requests

3 participants