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

Add support for GRASS 76 on Mac #9174

Merged
merged 1 commit into from
Feb 18, 2019
Merged

Add support for GRASS 76 on Mac #9174

merged 1 commit into from
Feb 18, 2019

Conversation

pmav99
Copy link
Contributor

@pmav99 pmav99 commented Feb 13, 2019

@luipir
Copy link
Contributor

luipir commented Feb 14, 2019

@pmav99 should it be backported?

@pmav99
Copy link
Contributor Author

pmav99 commented Feb 14, 2019

@luipir I guess it wouldn't hurt.
Should I do it? If yes, to which branch should it be added?

@pmav99
Copy link
Contributor Author

pmav99 commented Feb 14, 2019

BTW, now that I look at it better, it would probably make sense to reverse the list's ordering. I.e. use this:

for version in ['76', '74', '72', '71', '70', '7', '']:

instead of this:

for version in ['', '7', '70', '71', '72', '74', '76']:

Is it OK if I git push --force or should I make a separate commit?

@luipir
Copy link
Contributor

luipir commented Feb 14, 2019

make sense to reverse the list's ordering

IMHO make sense to get the highest version in case more than one version is installed. I'm not used to have multiple qgis version, so I do not exactly what would be the expected behaviour. If we change the logic we can introduce regressions. If we leave this logic a user that want the latest version will use the lowet version before and would be aware of this => forcing to remove the older one.

not clear to me what is the best solution. BTW We can invert the search logic without great risks to create regressions.

please do a separate commit to invert the list.

@PeterPetrik
Copy link
Contributor

maybe the hardcoded logic is not the best way how to handle this. I assume it would be easy to get all the directories starting with "grass" and using the one with the highest number (or alternatively the first one because I do not think you can have multiple of those)?

@luipir
Copy link
Contributor

luipir commented Feb 15, 2019

maybe the hardcoded logic is not the best way how to handle this. I assume it would be easy to get all the directories starting with "grass" and using the one with the highest number (or alternatively the first one because I do not think you can have multiple of those)?

@PeterPetrik I strongly agree... but we had cases where there here mutiple installation like "grass-7-svn"... there is no a standard way but sometimes could depend on packaging.
But IMHO this PR try to contiue following the path already available. e.g. a low impact. IMHO should be merged and leave to another PR to revrite the logic.

@m-kuhn
Copy link
Member

m-kuhn commented Feb 18, 2019

Looks good.
Let's merge this and anyone with a more flexible proposal can open a new pull request anytime.

@m-kuhn m-kuhn merged commit 9efe4c5 into qgis:master Feb 18, 2019
@m-kuhn
Copy link
Member

m-kuhn commented Feb 18, 2019

@pmav99 can you open a backport pull request?

@pmav99
Copy link
Contributor Author

pmav99 commented Feb 18, 2019

@m-kuhn sure, to which branch? release_3-4?

@m-kuhn
Copy link
Member

m-kuhn commented Feb 18, 2019

Exactly

@pmav99
Copy link
Contributor Author

pmav99 commented Feb 18, 2019

Hmm... This is git blame at release-3_4

It seems that the patch has been partially backported by @neteler with c022e1f but the list ordering has been changed. I think it would make sense to keep the same ordering both on LTS and on master. So which order should it be?

@neteler
Copy link
Contributor

neteler commented Feb 18, 2019

Please start with searching the newest GRASS GIS version first (I see no point in preferring older over newer versions)

lbartoletti added a commit to lbartoletti/QGIS that referenced this pull request Feb 18, 2019
+1 with @neteler
"start with searching the newest GRASS GIS version first"
qgis#9174 (comment)

cc  @rhurlin
nyalldawson pushed a commit that referenced this pull request Feb 20, 2019
+1 with @neteler
"start with searching the newest GRASS GIS version first"
#9174 (comment)

cc  @rhurlin
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.

5 participants