Makefile: Add the ability to create a source code release with all deps #104240

Sebastian Parborg · 2023-07-27T18:58:37+02:00

Sebastian Parborg commented

2023-07-27 18:58:37 +02:00

This adds two new recipes:

"only-deps": Download all the deps required to build the project offline
"release-package-source": Create a .tar.gz source release with all deps included

I'm not sure if you want this to be part of the "publish-release-packages" recipe, so I didn't touch that.

Perhaps we should also rename with-deps to something else?
oapi-codegen is already pulled in via go.mod.
It seems to me like all usage of mockgen is commented out.
The hugo dep also seem to only be for the project-website recipe?

This adds two new recipes: - "only-deps": Download all the deps required to build the project offline - "release-package-source": Create a .tar.gz source release with all deps included I'm not sure if you want this to be part of the "publish-release-packages" recipe, so I didn't touch that. Perhaps we should also rename `with-deps` to something else? `oapi-codegen` is already pulled in via `go.mod`. It seems to me like all usage of `mockgen` is commented out. The `hugo` dep also seem to only be for the ` project-website` recipe?

Sybren A. Stüvel commented

2023-07-28 14:26:14 +02:00

This adds two new recipes:

"only-deps": Download all the deps required to build the project offline

Since this is 'vendoring', and the Go command used is also go mod vendor, I think make vendor would be a better name.

"release-package-source": Create a .tar.gz source release with all deps included

👍

I'm not sure if you want this to be part of the "publish-release-packages" recipe, so I didn't touch that.

Yeah, everything that's part of 'the release' should be built by that command.

Perhaps we should also rename with-deps to something else?
oapi-codegen is already pulled in via go.mod.
It seems to me like all usage of mockgen is commented out.

Check the //go:generate lines in *.go to find where it's used.

The hugo dep also seem to only be for the project-website recipe?

It is, indeed.

> This adds two new recipes: > - "only-deps": Download all the deps required to build the project offline Since this is 'vendoring', and the Go command used is also `go mod vendor`, I think `make vendor` would be a better name. > - "release-package-source": Create a .tar.gz source release with all deps included :+1: > I'm not sure if you want this to be part of the "publish-release-packages" recipe, so I didn't touch that. Yeah, everything that's part of 'the release' should be built by that command. > Perhaps we should also rename `with-deps` to something else? > `oapi-codegen` is already pulled in via `go.mod`. > It seems to me like all usage of `mockgen` is commented out. Check the `//go:generate` lines in `*.go` to find where it's used. > The `hugo` dep also seem to only be for the ` project-website` recipe? It is, indeed.

Sebastian Parborg force-pushed vendor from 850464a927 to c3a133983c

2023-07-28 15:48:55 +02:00

Compare

Sebastian Parborg commented

2023-07-28 15:52:14 +02:00

I discovered that mockgen is already pulled in by mock in go.mod. So I moved the last dep to the project-website target as it is not used to build the default flamenco package (manager, worker, and addon).

I also noticed that google has dropped mockgen, but Uber as taken over the maintance, so the package url should probably updated in the future. (https://github.com/golang/mock)

I discovered that mockgen is already pulled in by `mock` in `go.mod`. So I moved the last dep to the `project-website` target as it is not used to build the default flamenco package (manager, worker, and addon). I also noticed that google has dropped mockgen, but Uber as taken over the maintance, so the package url should probably updated in the future. (https://github.com/golang/mock)

Sebastian Parborg force-pushed vendor from c3a133983c to 7bcce79085

2023-07-28 15:55:50 +02:00

Compare

Sybren A. Stüvel commented

2023-08-01 13:12:38 +02:00

I discovered that mockgen is already pulled in by mock in go.mod. So I moved the last dep to the project-website target as it is not used to build the default flamenco package (manager, worker, and addon).

Thinking about this again, this is not the right move. hugo is also used while working on the website, and that makefile rule is only used to publish the website. This means that you should be able to use hugo before running that rule.

I also noticed that google has dropped mockgen, but Uber as taken over the maintance, so the package url should probably updated in the future. (https://github.com/golang/mock)

👍

> I discovered that mockgen is already pulled in by `mock` in `go.mod`. So I moved the last dep to the `project-website` target as it is not used to build the default flamenco package (manager, worker, and addon). Thinking about this again, this is not the right move. `hugo` is also used while working on the website, and that makefile rule is only used to *publish* the website. This means that you should be able to use `hugo` before running that rule. > I also noticed that google has dropped mockgen, but Uber as taken over the maintance, so the package url should probably updated in the future. (https://github.com/golang/mock) :+1:

Sybren A. Stüvel requested changes 2023-08-01 14:05:36 +02:00

Sybren A. Stüvel left a comment

There are some issues with the generated source package:

A tarball should contain its contents in a subdirectory, so flamenco-3.3-alpha0-src.tar.gz should have a directory like flamenco-3.3-alpha0, which then contains all the current contents.
In the tarball the web/app/package-cache directory is empty.
After running this target, there is a vendor directory in the working directory. This makes the Go tooling behave differently (see go build docs about the -mod=vendor option), and gets in the way of future development.

There are some issues with the generated source package: - A tarball should contain its contents in a subdirectory, so `flamenco-3.3-alpha0-src.tar.gz` should have a directory like `flamenco-3.3-alpha0`, which then contains all the current contents. - In the tarball the `web/app/package-cache` directory is empty. - After running this target, there is a `vendor` directory in the working directory. This makes the Go tooling behave differently (see [go build docs](https://go.dev/ref/mod#build-commands) about the `-mod=vendor` option), and gets in the way of future development.

Makefile Outdated

						
				@ -54,2 +49,3 @@

					go mod vendor

				application: webapp flamenco-manager flamenco-worker

				application: webapp-deps flamenco-manager flamenco-worker

Sybren A. Stüvel commented

2023-08-01 13:14:15 +02:00

Shouldn't webapp-deps be a dependency of the webapp-static rule?

Shouldn't `webapp-deps` be a dependency of the `webapp-static` rule?

Sebastian Parborg commented

2023-08-01 16:29:27 +02:00

That is correct.
It will not work otherwise.

I've updated the make targets to reflect this.

That is correct. It will not work otherwise. I've updated the make targets to reflect this.

ZedDB marked this conversation as resolved

Makefile Outdated

						
				@ -356,0 +359,4 @@

					rm -f dist/${RELEASE_SOURCE_ARCHIVE_GZ}

					git archive -o dist/${RELEASE_SOURCE_ARCHIVE} HEAD

					tar -rvf dist/${RELEASE_SOURCE_ARCHIVE} vendor web/app/package-cache

					gzip dist/${RELEASE_SOURCE_ARCHIVE}

Sybren A. Stüvel commented

2023-08-01 13:19:40 +02:00

What's the reason to use two separate commands, and keep two separate files, for the .tar and the .tar.gz?

I think it's better to switch from GZip to a more modern compression standard, and to compress in one go, with tar -rvJf and set RELEASE_SOURCE_ARCHIVE := flamenco-${VERSION}-src.tar.xz

What's the reason to use two separate commands, and keep two separate files, for the `.tar` and the `.tar.gz`? I think it's better to switch from GZip to a more modern compression standard, and to compress in one go, with `tar -rvJf` and set `RELEASE_SOURCE_ARCHIVE := flamenco-${VERSION}-src.tar.xz`

Sebastian Parborg commented

2023-08-01 16:34:06 +02:00

The reason is that you can not append un-tracked folders with git archive.
So I do this in two steps so that we only create a source archive will all tracked files plus the two vendoring folders.
However the gzip command will not keep the original .tar file so you will only end up with one .tar.gz file.

The reason I used .tar.gz is that it seems to be the default source archive format on all big git file hosts (github, gitlab, and gitea).

Of course we can change this to xz if you wish.

The reason is that you can not append un-tracked _folders_ with `git archive`. So I do this in two steps so that we only create a source archive will all tracked files plus the two vendoring folders. However the `gzip` command will not keep the original `.tar` file so you will only end up with one `.tar.gz` file. The reason I used .tar.gz is that it seems to be the default source archive format on all big git file hosts (github, gitlab, and gitea). Of course we can change this to xz if you wish.

Sebastian Parborg commented

2023-08-01 16:43:18 +02:00

After running this target, there is a vendor directory in the working directory. This makes the Go tooling behave differently (see go build docs about the -mod=vendor option), and gets in the way of future development.

There doesn't seem to be a nice way to check if the user has the vendor directory already and not remove it after running that recipe.
However if you want to unconditionally remove it after packaging is done, we can do that.

> - After running this target, there is a `vendor` directory in the working directory. This makes the Go tooling behave differently (see [go build docs](https://go.dev/ref/mod#build-commands) about the `-mod=vendor` option), and gets in the way of future development. > There doesn't seem to be a nice way to check if the user has the `vendor` directory already and not remove it after running that recipe. However if you want to unconditionally remove it after packaging is done, we can do that.

Sebastian Parborg commented

2023-08-01 16:45:51 +02:00

This means that you should be able to use hugo before running that rule.

I've created a new standalone rule for it so if one wants to install the dependencies to work on the project website, it is now easy to do so.

> This means that you should be able to use hugo before running that rule. I've created a new standalone rule for it so if one wants to install the dependencies to work on the project website, it is now easy to do so.

Sybren A. Stüvel commented

2023-08-23 15:27:23 +02:00

I've created a new standalone rule for it so if one wants to install the dependencies to work on the project website, it is now easy to do so.

IMO changing this is out of scope for this PR. In current main there is just one make command to use to install all the dependencies (except for Go & NodeJS themselves). This ease of getting started with development is important, and having to figure out which part of the work requires which initialisation steps goes against that.

Having a way to package the dependencies (as per the PR title) shouldn't impact how people are onboarded into the project.

> I've created a new standalone rule for it so if one wants to install the dependencies to work on the project website, it is now easy to do so. IMO changing this is out of scope for this PR. In current `main` there is just one `make` command to use to install all the dependencies (except for Go & NodeJS themselves). This ease of getting started with development is important, and having to figure out which part of the work requires which initialisation steps goes against that. Having a way to package the dependencies (as per the PR title) shouldn't impact how people are onboarded into the project.

Sebastian Parborg commented

2023-09-08 14:43:43 +02:00

This ease of getting started with development is important, and having to figure out which part of the work requires which initialisation steps goes against that.
Having a way to package the dependencies (as per the PR title) shouldn't impact how people are onboarded into the project.

Now I'm confused, these changes doesn't impact how people are on-boarded or makes them jump through hoops to figure out which target the need to make.

This instead simplifies it so that they don't even have to run any prerequisite make targets as now everything properly depends on the targets they need. So I would strongly argue that this makes onboarding and building the the whole project or individual parts much easier.

This wasn't the case before, at least for the website. Before the user must have known to run the with-deps otherwise building the project-website target would fail. Now they can just run make project-website and everything it needs deps wise is pulled in automatically.

> This ease of getting started with development is important, and having to figure out which part of the work requires which initialisation steps goes against that. > Having a way to package the dependencies (as per the PR title) shouldn't impact how people are onboarded into the project. Now I'm confused, these changes doesn't impact how people are on-boarded or makes them jump through hoops to figure out which target the need to make. This instead simplifies it so that they don't even have to run any prerequisite make targets as now everything properly depends on the targets they need. So I would strongly argue that this makes onboarding and building the the whole project or individual parts much easier. This wasn't the case before, at least for the website. Before the user must have known to run the `with-deps` otherwise building the `project-website` target would fail. Now they can just run `make project-website` and everything it needs deps wise is pulled in automatically.

Sybren A. Stüvel commented

2023-09-08 15:00:30 +02:00

Now I'm confused, these changes doesn't impact how people are on-boarded or makes them jump through hoops to figure out which target the need to make.

You moved the installation of Hugo from the regular dependencies to the rule that deploys the website to the Blender infrastructure.

Anybody working on the documentation will need to be able to self-host the website, and thus be able to run Hugo. Not just the two people on the planet who deploy the actual website to the production server.

> Now I'm confused, these changes doesn't impact how people are on-boarded or makes them jump through hoops to figure out which target the need to make. You moved the installation of Hugo from the regular dependencies to the rule that *deploys* the website to the Blender infrastructure. Anybody working on the documentation will need to be able to self-host the website, and thus be able to run Hugo. Not just the two people on the planet who deploy the actual website to the production server.

Sebastian Parborg force-pushed vendor from e3e6283296 to bdc63b3057

2023-09-08 15:53:57 +02:00

Compare

Sebastian Parborg commented

2023-09-08 15:55:33 +02:00

We talked a bit offline and made sure we were on the same page on all points.

I've forced pushed the rebased patches and dropped the latest change that wasn't what we actually wanted to change.

We talked a bit offline and made sure we were on the same page on all points. I've forced pushed the rebased patches and dropped the latest change that wasn't what we actually wanted to change.

Sybren A. Stüvel requested changes 2023-09-26 18:00:41 +02:00

Sybren A. Stüvel left a comment

The way the Makefile is structured now will always run yarn install --force, also for situations where you don't want to build a source package. I don't think that's the right approach. Regular builds should be as fast as possible, as build time has a direct impact on iteration speed of developers.

The removal of the with-deps target means that the developer documentation is no longer correct. This should be addressed in this commit as well.

The way the Makefile is structured now will always run `yarn install --force`, also for situations where you don't want to build a source package. I don't think that's the right approach. Regular builds should be as fast as possible, as build time has a direct impact on iteration speed of developers. The removal of the `with-deps` target means that the developer documentation is no longer correct. This should be addressed in this commit as well.

Makefile

						
				@ -96,2 +93,2 @@

				webapp:

					yarn --cwd web/app install

				webapp-deps:

					# We have to use --force here to ensure that the offline package cache is up to date with no missing files

Sybren A. Stüvel commented

2023-09-26 17:51:11 +02:00

This comment should be hidden from the regular output. Right now it's actually shown when running make webapp-deps (and any other target that runs this target).

This comment should be hidden from the regular output. Right now it's actually shown when running `make webapp-deps` (and any other target that runs this target).

Makefile

						
				@ -332,2 +330,4 @@

				RELEASE_PACKAGE_SHAFILE := flamenco-${VERSION}.sha256

				RELEASE_SOURCE_ARCHIVE := flamenco-${VERSION}-src.tar

				RELEASE_SOURCE_ARCHIVE_GZ := flamenco-${VERSION}-src.tar.gz

Sybren A. Stüvel commented

2023-09-26 17:59:11 +02:00

I think it's better to define RELEASE_SOURCE_ARCHIVE_GZ as ${RELEASE_SOURCE_ARCHIVE}.gz

I think it's better to define `RELEASE_SOURCE_ARCHIVE_GZ` as `${RELEASE_SOURCE_ARCHIVE}.gz`

Makefile

						
				@ -380,0 +383,4 @@

					mkdir -p dist

					rm -f dist/${RELEASE_SOURCE_ARCHIVE_GZ}

					git archive --prefix=flamenco-${VERSION}/ -o dist/${RELEASE_SOURCE_ARCHIVE} HEAD

					tar --transform 's,^,flamenco-${VERSION}/,' -rvf dist/${RELEASE_SOURCE_ARCHIVE} vendor web/app/package-cache

Sybren A. Stüvel commented

2023-09-26 18:00:16 +02:00

What's the reason to use gzip separately? Why not just use tar z?

What's the reason to use `gzip` separately? Why not just use `tar z`?

web/app/.yarnrc

						
				@ -0,0 +1,2 @@

				yarn-offline-mirror "./package-cache"

				yarn-offline-mirror-pruning true

Sybren A. Stüvel commented

2023-09-26 17:58:08 +02:00

Instead of always caching the packages, put this into a special file .yarnrc-source-pkg or something, and specify that when running yarn install for grabbing the sources. That way the regular builds don't have to bother with this package cache.

Instead of always caching the packages, put this into a special file `.yarnrc-source-pkg` or something, and specify that when running `yarn install` for grabbing the sources. That way the regular builds don't have to bother with this package cache.

This pull request has changes conflicting with the target branch.