search

neanes / neanes

Neanes is a free and open source scorewriter for notating Byzantine chant in Byzantine notation.
https://neanes.github.io/neanes/
GNU General Public License v3.0
35 stars 9 forks source link

`README.md` should describe how to perform reproducible builds #92

Closed basil closed 1 year ago

basil commented 1 year ago

Without this PR, following the development instructions leads to a checkout where git status is not clean and git commit -a commits unrelated changes to package-lock.json, which would presumably fail code review.

Tested with:

$ git clean -fxd
$ git status  # clean
$ npm ci
$ git status  # clean
$ npm test
$ npm run electron:build
$ ./dist_electron/Neanes-0.3.0-beta.19.AppImage  # success

Fixes #99.

danielgarthur commented 1 year ago

My expectation would be that developers use npm install. If this results in a change to package-lock.json, then this would committed as part of the PR. But there should never be a PR that is purely to update package-lock.json. I would consider this standard practice, although I'm sure opinions vary.

If for some reason, a developer wants to use npm ci, they can use that, but it's not required.

If the goal is to keep package-lock dependencies more up-to-date, we could look at using dependabot or something similar.

basil commented 1 year ago

My expectation would be that developers use npm install

But that would result in the developers using a different set of transitive dependencies than the CI build, thus breaking reproducibility. And it would break git commit -a as I described in the PR description.

If for some reason, a developer wants to use npm ci, they can use that, but it's not required.

What do you mean by "for some reason"? I gave the reasons in my PR description: reproducibility and a clean git status, which should be expected of all Git projects. It should not only be used by developers who are not updating package.json, but it should be preferred for the reasons given above, hence this PR to update the documentation.

danielgarthur commented 1 year ago

In a decade of software development, I've never had an issue with npm install during development causing builds to break.

basil commented 1 year ago

That is an argument from authority, not explicit reasoning.

In a decade of software development, I've never seen a serious project where git status wasn't clean after compiling and running the project according to the README. :man_shrugging:

danielgarthur commented 1 year ago

When bumping versions, I haven't been consistently updating the package version in the package-lock.json, which I should be doing.

When I run npm install locally, the only changes I get are related to updating that version number. I'm all for fixing that. I noticed the package-lock in the other PR has a lot of other changes in it, though, which are not expected. Where are those changes coming from?

basil commented 1 year ago

When bumping versions, I haven't been consistently updating the package version in the package-lock.json, which I should be doing.

That would be an indication that you favor #91 as opposed to #92 as a solution to #99. If #91 is accepted, the follow-up PR promised in https://github.com/danielgarthur/neanes/pull/91#issuecomment-1550317173 would enforce in CI that package-lock.json is always updated in lockstep with package.json, so you wouldn't have to worry about forgetting this in the future.

When I run npm install locally, the only changes I get are related to updating that version number. I'm all for fixing that. I noticed the package-lock in the other PR has a lot of other changes in it, though, which are not expected. Where are those changes coming from?

Feedback that would be better left on the other PR. But in short, I didn't make these changes manually. They were made automatically after I ran npm install, so any differences must be a function of the version of Node and NPM we have installed on our respective build environments. Mine is a rather plain Ubuntu Linux desktop, the latest stable Node 16.x LTS release from https://nodejs.org/en/download, and whatever NPM comes bundled with that. (In https://github.com/danielgarthur/neanes/pull/91#issuecomment-1550317173 we would define the canonical build environment as whatever is used by the CI job.)

basil commented 1 year ago

The problem can also be reproduced in Docker (amd64) with:

$ docker run --rm -it node:16.20.0 /bin/bash -c 'git clone --depth 1 https://github.com/danielgarthur/neanes.git && cd neanes && npm install && git diff'

Same diff as from #91.

danielgarthur commented 1 year ago

Closing in favor of #91.