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 option 'update' #7

Merged
merged 1 commit into from
Oct 21, 2019
Merged

add option 'update' #7

merged 1 commit into from
Oct 21, 2019

Conversation

eine
Copy link
Contributor

@eine eine commented Oct 9, 2019

Currently, uname -a is executed by default after extracting MSYS2. The purpose of executing it is to initialize the environment, so it is not delayed until the user calls the first command.

This PR adds option/parameter update, which will replace uname -a with pacman -Syu --noconfirm. On the one hand, this makes the initialization call actually useful, instead of being "dummy". On the other hand, updating the environment allows to use packages which were published after the tarball was released/cached.

Corresponding tests are added to the GHA worklow, and the README is updated.

@eine
Copy link
Contributor Author

eine commented Oct 20, 2019

ping @Ecco...

@Ecco
Copy link
Contributor

Ecco commented Oct 20, 2019

Well, is this really needed? It seems like this is a bit bloated…

I mean, if you want to do pacman whatever, then just add a step msys2do pacman whatever 😄

I don't think we need to bother doing this at the setup-msys2 level.

@eine
Copy link
Contributor Author

eine commented Oct 20, 2019

The main purpose is to do something actually useful instead of uname -a. Maybe do pacman -Syu --noconfirm by default (with no option/parameter)?

@Ecco
Copy link
Contributor

Ecco commented Oct 21, 2019

The main purpose is to do something actually useful instead of uname -a.

Well, we could as well do… nothing 😄 The point of uname -a is just to pre-heat msys2, but we might as well do nothing, couldn't we?

Maybe do pacman -Syu --noconfirm by default (with no option/parameter)?

Well, I thought about this and didn't do it for two reasons:

  1. Reproducibility: an updated package might break something in the future. Not updating pacman freezes the packages, which is a great feature IMHO.
  2. It's just slower. If a third party doesn't care, then we're just slowing down their CI for nothing.

@eine
Copy link
Contributor Author

eine commented Oct 21, 2019

I might change it from update to init and leave it empty by default. Then, it's up to the users to do nothing, uname -a, pacman -Syu --noconfirm or whatever. wdyt?

@Ecco
Copy link
Contributor

Ecco commented Oct 21, 2019

Well, my opinion is that it's better to not introduce extra config options for things which can easily be done on a case-by-case basis. And I think it's easy to just add a line which says - run: msys2do pacman -Syu --noconfirm on project that would need this 😄

But if you think it's really useful for a lot of users, then I'm ok with merging it as-is, though. Don't bother with a half-assed "init" alternative 😄

@eine
Copy link
Contributor Author

eine commented Oct 21, 2019

My use case is that I want to have both options: test building the stable version of a tool with the stable MSYS2 distribution (to generate release artifacts); and test the master version of the tool with the latest MSYS2 distribution to find potential issues early. I believe that many users might want to do the same.

I'd like to avoid adding one or two explicit (conditional) steps for that, because I consider it to be part of the environment setup, not of the workflow (build, test, deploy). Since it is actually 4 lines of code, I think it is not much effort to maintain it (although 15 lines of tests are added).

@Ecco Ecco merged commit 3032d4b into numworks:master Oct 21, 2019
@Ecco
Copy link
Contributor

Ecco commented Oct 21, 2019

Fair enough 😄

@eine eine deleted the feat-update branch October 21, 2019 21:13
@eine eine mentioned this pull request Nov 25, 2019
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.

2 participants