Bug 32609 - Remove compiled files from src
Summary: Remove compiled files from src
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL: https://gitlab.com/joubu/Koha/-/commi...
Keywords:
Depends on:
Blocks: 32806 32975 32994
  Show dependency treegraph
 
Reported: 2023-01-11 15:27 UTC by Jonathan Druart
Modified: 2024-02-05 05:54 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This important architectural change removes the built CSS and JavaScript files from source control and introduces a build process and trigger into our packaging routines. This will both save space in the repository and lead to less mistakes from developers by dropping the need to build, add and commit these files at release time.
Version(s) released in:
23.05.00, 22.11.04


Attachments
Bug 32609: Remove JS vars from gulpfile (1.00 KB, patch)
2023-01-11 16:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32609: Improve yarn build commands (4.26 KB, patch)
2023-01-11 16:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32609: Add compiled css files to gitignore (983 bytes, patch)
2023-01-11 16:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32609: Add warning to the about page (3.49 KB, patch)
2023-01-11 16:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32609: (follow-up) Add build-resources.PL (2.17 KB, patch)
2023-01-30 16:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32609: (follow-up) enhance Makefile.PL (4.21 KB, patch)
2023-01-31 02:33 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-01-11 15:27:00 UTC
We need to improve our yarn build commands to generate css and js compiled files.

Then remove them from our codebase, and generate them when needed:

- when generating the debian packages
- when ktd is starting up
- when patches are applied on the sandbox
Comment 1 Jonathan Druart 2023-01-11 16:13:15 UTC
Created attachment 145209 [details] [review]
Bug 32609: Remove JS vars from gulpfile

They are not used.
Comment 2 Jonathan Druart 2023-01-11 16:13:19 UTC
Created attachment 145210 [details] [review]
Bug 32609: Improve yarn build commands

We had:
yarn css # Build staff css
yarn css --view opac # Build OPAC css
yarn build # Nothing, was broken
yarn watch # Nothing, was broken
yarn build_js # Build JS/Vue
yarn watch_js # Watch JS/Vue

This patch is suggesting to have something more consistent:
yarn css:build      # Build CSS for OPAC and staff (dev)
yarn css:build:prod # Build CSS for OPAC and staff (prod)
yarn css:watch      # Watch CSS for OPAC and staff (dev)
yarn js:build       # Build JS (dev)
yarn js:build:prod  # Build JS (prod)
yarn js:watch       # Watch JS (dev)
yarn build          # Same as css:build and js:build (dev)
yarn build:prod     # Same as css:build:prod and js:build:prod (prod)

Note that the cssnano was breaking the build function.
Comment 3 Jonathan Druart 2023-01-11 16:13:24 UTC
Created attachment 145211 [details] [review]
Bug 32609: Add compiled css files to gitignore
Comment 4 Jonathan Druart 2023-01-11 16:14:20 UTC
Created attachment 145212 [details] [review]
Bug 32609: Add warning to the about page
Comment 5 Jonathan Druart 2023-01-11 16:15:01 UTC
And the last patch didn't make it, too large.
Comment 6 Jonathan Druart 2023-01-11 16:16:09 UTC
I think this is going toward the good direction, what do you think?
Comment 7 Jonathan Druart 2023-01-11 16:16:50 UTC
The full patch set is on a remote branch: https://gitlab.com/joubu/Koha/-/commits/bug_32609
Comment 8 Tomás Cohen Arazi 2023-01-11 16:21:47 UTC
+1
Comment 9 Katrin Fischer 2023-01-11 18:17:36 UTC
(In reply to Jonathan Druart from comment #0)
> We need to improve our yarn build commands to generate css and js compiled
> files.
> 
> Then remove them from our codebase, and generate them when needed:
> 
> - when generating the debian packages
> - when ktd is starting up
> - when patches are applied on the sandbox

Hm, what about tarballs? 

We roll our own packages - what would be the suggested workflow? Generate the files on the git branch before building the packages?
Comment 10 Mason James 2023-01-12 13:04:20 UTC
(In reply to Katrin Fischer from comment #9)
> (In reply to Jonathan Druart from comment #0)
> > We need to improve our yarn build commands to generate css and js compiled
> > files.
> > 
> > Then remove them from our codebase, and generate them when needed:
> > 
> > - when generating the debian packages
> > - when ktd is starting up
> > - when patches are applied on the sandbox
> 
> Hm, what about tarballs? 
> 
> We roll our own packages - what would be the suggested workflow? Generate
> the files on the git branch before building the packages?

i think we might be able to add these steps to Makefile.PL, so they get run for both tarball and deb pkg builds
Comment 11 David Cook 2023-01-15 23:53:10 UTC
(In reply to Mason James from comment #10)
> (In reply to Katrin Fischer from comment #9)
> > Hm, what about tarballs? 
> > 
> > We roll our own packages - what would be the suggested workflow? Generate
> > the files on the git branch before building the packages?
> 
> i think we might be able to add these steps to Makefile.PL, so they get run
> for both tarball and deb pkg builds

Yeah I reckon we add something to PL_FILES: https://metacpan.org/pod/ExtUtils::MakeMaker#PL_FILES

That way it would apply to tarballs, deb pkg builds, and technically even git dev installs.
Comment 12 David Cook 2023-01-16 00:02:00 UTC
(In reply to Jonathan Druart from comment #0)
> We need to improve our yarn build commands to generate css and js compiled
> files.
> 
> Then remove them from our codebase, and generate them when needed:
> 
> - when generating the debian packages
> - when ktd is starting up
> - when patches are applied on the sandbox

I suppose "ktd" startup time makes sense. On one hand, I might like to generate the files 1 time, and re-use them over days/weeks/months, but... having a fresh build on each startup is probably wise. 

When I switch version branches, I'll destroy my container and rebuild it, so this would work well with that workflow. 

And if you change branches that have the same version but changes to css and js... you just need to re-compile anyway. 

So yeah that sounds good to me. 

--

If we create a Perl script to run these commands, we can include it in Makefile.PL, plus manually call it for "ktd" and sandboxes, so... that should be pretty straightforward overall.
Comment 13 Martin Renvoize 2023-01-17 15:35:10 UTC
This is looking great.. we clearly need to continue down this route in my opinion.

Let me know how I can help.. obviously it will require changes in surrounding projects like ktd, sandboxes, packaging pipeline.. we should be able to split the load at least a little.
Comment 14 Fridolin Somers 2023-01-17 21:46:53 UTC
Makes completely sense, I'm in :D
Comment 15 Jonathan Druart 2023-01-24 16:11:17 UTC
(In reply to David Cook from comment #12)
> If we create a Perl script to run these commands, we can include it in
> Makefile.PL, plus manually call it for "ktd" and sandboxes, so... that
> should be pretty straightforward overall.

Actually it's not obvious at all for me how this can be done easily.

There are dev paths in gulpfile...

We could rely no an env var (or an option?) that would pass the path.

Like:
  css_path=/usr/share/koha/intranet/htdocs/intranet-tmpl/prog/css/ yarn css:build:prod

I've done this already.
For the js/vue dist file, it's a bit more tricky (node path, relative js path, modules, etc.)
Comment 16 Jonathan Druart 2023-01-24 16:16:19 UTC
2 new commits on the remote branch

Bug 32609: Rely no env var to generate js files
Bug 32609: Rely on env var to generate css files

The CSS one is working, the JS is not. It's failing with:

ERROR in ../../usr/share/koha/intranet/htdocs/intranet-tmpl/prog/js/vue/main-erm.ts 3:12-26                                                                                                                          
Module not found: Error: Can't resolve 'vue' in '/usr/share/koha/intranet/htdocs/intranet-tmpl/prog/js/vue'    

and many more.
Comment 17 David Cook 2023-01-24 22:32:55 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to David Cook from comment #12)
> > If we create a Perl script to run these commands, we can include it in
> > Makefile.PL, plus manually call it for "ktd" and sandboxes, so... that
> > should be pretty straightforward overall.
> 
> Actually it's not obvious at all for me how this can be done easily.
> 
> There are dev paths in gulpfile...
> 
> We could rely no an env var (or an option?) that would pass the path.
> 
> Like:
>   css_path=/usr/share/koha/intranet/htdocs/intranet-tmpl/prog/css/ yarn
> css:build:prod
> 
> I've done this already.
> For the js/vue dist file, it's a bit more tricky (node path, relative js
> path, modules, etc.)

In my mind, you would only be running the build from the source directory and not a deployed directory, so the paths should always be those dev paths.

We should just need to add a script to Koha for running the JS build, and then for ktd and sandboxes, we invoke it (targeting the git checkouts) via their respective toolchains. We add a call to that script in Makefile.PL as well to build the JS for single/standard/dev installs (and package installs which use the standard install implicitly). 

--

Of course, I might be missing something here. 

If you can tell me which commands we need to build the JS and CSS, I'm happy to give this a try...
Comment 18 Jonathan Druart 2023-01-26 10:30:03 UTC
(In reply to David Cook from comment #17)
> If you can tell me which commands we need to build the JS and CSS, I'm happy
> to give this a try...

It's in commit message of the second commit:

    This patch is suggesting to have something more consistent:
    yarn css:build      # Build CSS for OPAC and staff (dev)
    yarn css:build:prod # Build CSS for OPAC and staff (prod)
    yarn css:watch      # Watch CSS for OPAC and staff (dev)
    yarn js:build       # Build JS (dev)
    yarn js:build:prod  # Build JS (prod)
    yarn js:watch       # Watch JS (dev)
    yarn build          # Same as css:build and js:build (dev)
    yarn build:prod     # Same as css:build:prod and js:build:prod (prod)

Here we will need `yarn build:prod` that will generate the minified css files and the dist file for the Vue app.
Comment 19 Martin Renvoize 2023-01-30 16:17:44 UTC
Created attachment 145802 [details] [review]
Bug 32609: (follow-up) Add build-resources.PL

This patch adds a trivial perl wrapper around the yarn build and calls
it from Makefile.PL.

I 'think' this is all we need to get packaging, dev and standard
installs happy with the lack of our compiled files in source.
Comment 20 Martin Renvoize 2023-01-30 16:19:58 UTC
My patch is on top of Jonathans branch (though with it I think we don't need his last two patches).

I've not actually tested it in the end to end sense of building a package.  Also, it assumes dependencies for yarn and friends are there.. but I think that's a fair assumption for a build box right?

Let me know if we're thinking along the same lines David and please feel free to improve further.
Comment 21 David Cook 2023-01-31 00:18:46 UTC
(In reply to Martin Renvoize from comment #20)
> Let me know if we're thinking along the same lines David and please feel
> free to improve further.

I'm thinking along the same lines although I don't think it'll work as expected at present.

I think that I can get this to work though. Patch incoming...
Comment 22 David Cook 2023-01-31 00:38:21 UTC
If we're going to get a script file from PL_FILES to run "before" pm_to_blib, then we need to use a target that can be found in "PM". 

Since Jonathan removed the files, "perl Makefile.PL" creates a Makefile that doesn't include the CSS and JS targets in "PM", so they don't get copied into "blib" and don't wind up in the tarball (for tarball/package installs). 

If we're going to use Makefile.PL to include generated files, we just need to add them to the "PM" argument in Makefile.PL and to call build-resources.PL with an argument that can be found in "PM". (We don't want multiple arguments, or build-resources.PL will be run multiple times.)
Comment 23 David Cook 2023-01-31 00:38:51 UTC
Currently having this issue:

root@kohadevbox:koha(bug_32609)$ make
"/usr/bin/perl" build-resources.PL koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js
yarn run v1.22.19
$ yarn css:build:prod && yarn js:build:prod
$ gulp build && gulp css --view opac
[00:35:54] Local modules not found in /kohadevbox/koha
[00:35:54] Try running: yarn install
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Comment 24 David Cook 2023-01-31 00:40:35 UTC
According to https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_interface, I first need to run "yarn install"

I wonder if that should be in build-resources.PL or not.

I'll add it in and see how I go...
Comment 25 David Cook 2023-01-31 01:08:03 UTC
With my (yet to be attached) patch, I run the following:

1. perl Makefile.PL
#NOTE: On the first run, 
2. make
3. diff -q -r koha-tmpl/intranet-tmpl/ blib/INTRANET_TMPL_DIR/
#NOTE: No output
4. diff -q -r koha-tmpl/opac-tmpl blib/OPAC_TMPL_DIR/
#NOTE: No output

That means that the CSS and JS files have been built and copied across to "blib". From there, they can be installed using a source install or built into a Debian package.
Comment 26 David Cook 2023-01-31 01:11:31 UTC
(In reply to David Cook from comment #25)
> That means that the CSS and JS files have been built and copied across to
> "blib". From there, they can be installed using a source install or built
> into a Debian package.

I verified this by running "make install" in a "single" deployment.

/opt/koha/intranet/htdocs/intranet-tmpl/prog/js/vue/dist/main.js
/opt/koha/opac/htdocs/opac-tmpl/bootstrap/css/opac.css

You could manually verify the rest. There's just a bunch of generated files to list.
Comment 27 David Cook 2023-01-31 01:25:42 UTC
However, I'm noticing a problem... at the moment I'm hooking off "./koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js". If that file exists in the source directory, then the yarn build won't run when running "make".

That shouldn't be a problem for a tarball/package install... since the generated files won't exist... but it wouldn't work for a git dev install. However, as we noted before, we're not really catering to that. 

So a git dev install, ktd, and sandboxes would need to run "build-resources.PL" manually to update the JS and CSS.

--

But I'm going to keep playing as I have a couple more ideas...
Comment 28 David Cook 2023-01-31 02:33:42 UTC
Created attachment 145838 [details] [review]
Bug 32609: (follow-up) enhance Makefile.PL

This patch adds a new target to the Makefile which gets
called as the first step of the "all" target. This means
that CSS and JS files get compiled every time "make" is
called.

(Note: Since the "install" target depends on "all", it will
 be re-run at install time.)
Comment 29 David Cook 2023-01-31 02:42:12 UTC
Ok, so I've fixed it, so that the CSS and JS get compiled every time "make"[1] is called, so they'll always be updated for source/package installs (which use "make" and "make install") and git dev installs (if they use "make" and "make upgrade").

For "ktd" and sandboxes (and git dev installs that don't use "make"), just call "build-resources.PL" to compile the CSS and JS.

--

My patch updates the Makefile so that the "all" target relies on a new "compile_css_js" target which runs build-resources.PL. That's fairly elegant.

The only non-elegant part is needing to list all the compiled files in Makefile.PL so that they get correctly copied into the "blib" directory at build time.

That code could be further streamlined to reduce boilerplate and the chance of human error, although ultimately we will need to provide a list of generated files at "perl Makefile.PL" time, or else the generated files won't get deployed. In theory, this list should not change often.

--

[1] Note that someone many many years made the "install" target depend on "all", so "make install" actually re-runs "make" effectively.
Comment 30 David Cook 2023-01-31 02:45:00 UTC
Anyway, I'm pretty happy with how that turned out overall. It certainly does the trick.
Comment 31 Jonathan Druart 2023-01-31 07:59:18 UTC
Thanks Martin and David, I've removed my patches and added yours to the remote branch.

I have one concern about the `yarn install` step. At some point we want to freeze the node_modules directory, or we may get expected regressions at a very bad time.
Imagine we are testing a beta version D-15 before the release, everything is working great, we decide to release on D-day, package maintainer releases using `yarn install && yarn build:prod`. If something changed in the node modules, we haven't tested it and we release a broken version.

It's tricky and I don't have a solution right now, but we need to:
* Let developers yarn install whenever they want (/kohadevbox/koha/node_modules will be used/tested)
* Have a generated (when?) version (/kohadevbox/node_modules) that would be used to generate the compiled files on ktd startup (sandboxes and devs). We could run build:prod to avoid last minute bugs to popup.
* Pick the last generated version for the release
Comment 32 Martin Renvoize 2023-01-31 11:59:49 UTC
Indeed, that's why I was hesitant to put the 'yarn install' within the PL.  I'm sure there must be something we can pass there to ensure it installed pinned versions from our committed lock file.. but I'm damned if I can spot such an option.
Comment 33 David Cook 2023-02-01 01:41:44 UTC
(In reply to Martin Renvoize from comment #32)
> Indeed, that's why I was hesitant to put the 'yarn install' within the PL. 
> I'm sure there must be something we can pass there to ensure it installed
> pinned versions from our committed lock file.. but I'm damned if I can spot
> such an option.

I think "yarn install --frozen-lockfile" might do the trick?
Comment 34 David Cook 2023-02-01 02:02:13 UTC
(In reply to Jonathan Druart from comment #31)
> I have one concern about the `yarn install` step. At some point we want to
> freeze the node_modules directory, or we may get expected regressions at a
> very bad time.
> Imagine we are testing a beta version D-15 before the release, everything is
> working great, we decide to release on D-day, package maintainer releases
> using `yarn install && yarn build:prod`. If something changed in the node
> modules, we haven't tested it and we release a broken version.

If we use "yarn install --frozen-lockfile" when running via Makefile.PL, that should avoid that scenario. We'd be using only the versions from yarn.lock that hypothetically were tested.

That said, testing will be interesting. I suppose you'll get generated files at ktd startup time, and those should be OK until you change branches. At that point, you might have the wrong generated files and not know it. 

When it comes to local dev/test, we might need to just get into the habit of being mindful of doing yarn install/build when we switch branches (if required).

Sandboxes shouldn't be a problem since we could automate the file re-generation...

> It's tricky and I don't have a solution right now, but we need to:
> * Let developers yarn install whenever they want
> (/kohadevbox/koha/node_modules will be used/tested)
> * Have a generated (when?) version (/kohadevbox/node_modules) that would be
> used to generate the compiled files on ktd startup (sandboxes and devs). We
> could run build:prod to avoid last minute bugs to popup.
> * Pick the last generated version for the release

I suppose test/QA theoretically should catch if there's a problem with generated files. So the "last generated version for the release" will just be what's in the package.json and yarn.lock files. 

Maybe we're overthinking it a bit?

Although I think changing "yarn install" to "yarn install --frozen-lockfile" sounds necessary for sure.
Comment 35 Jonathan Druart 2023-02-01 07:45:06 UTC
(In reply to David Cook from comment #34)
> (In reply to Jonathan Druart from comment #31)
> > I have one concern about the `yarn install` step. At some point we want to
> > freeze the node_modules directory, or we may get expected regressions at a
> > very bad time.
> > Imagine we are testing a beta version D-15 before the release, everything is
> > working great, we decide to release on D-day, package maintainer releases
> > using `yarn install && yarn build:prod`. If something changed in the node
> > modules, we haven't tested it and we release a broken version.
> 
> If we use "yarn install --frozen-lockfile" when running via Makefile.PL,
> that should avoid that scenario. We'd be using only the versions from
> yarn.lock that hypothetically were tested.

Indeed, we discussed that already but forgot. Good :)

> That said, testing will be interesting. I suppose you'll get generated files
> at ktd startup time, and those should be OK until you change branches. At
> that point, you might have the wrong generated files and not know it. 
> 
> When it comes to local dev/test, we might need to just get into the habit of
> being mindful of doing yarn install/build when we switch branches (if
> required).

Or use watch :)

> Sandboxes shouldn't be a problem since we could automate the file
> re-generation...
> 
> > It's tricky and I don't have a solution right now, but we need to:
> > * Let developers yarn install whenever they want
> > (/kohadevbox/koha/node_modules will be used/tested)
> > * Have a generated (when?) version (/kohadevbox/node_modules) that would be
> > used to generate the compiled files on ktd startup (sandboxes and devs). We
> > could run build:prod to avoid last minute bugs to popup.
> > * Pick the last generated version for the release
> 
> I suppose test/QA theoretically should catch if there's a problem with
> generated files. So the "last generated version for the release" will just
> be what's in the package.json and yarn.lock files. 
> 
> Maybe we're overthinking it a bit?

I did overthink it yesterday, yes! Glad we have an easy solution. The question is then when do we run `yarn install` and update the lock file? But we can discuss that later.
Comment 36 Jonathan Druart 2023-02-01 07:49:04 UTC
New commit added to the remote branch: "Bug 32609: Use the current yarn.lock to generate node_modules"
Comment 37 Jonathan Druart 2023-02-01 07:51:53 UTC
This is ready to be tested!
Comment 38 Martin Renvoize 2023-02-01 08:07:50 UTC
Awesome teamwork here chaps.. I'll resume testing today, should be able to get a signoff done.

Do we need a list of other tasks that need taking care as part of pushing this.. i.e. we'll need to update the sandboxes and ktd to run the new yarn commands (or perhaps point them at the new build_resources.PL script alternatively).
Comment 39 Jonathan Druart 2023-02-01 08:39:45 UTC
(In reply to Martin Renvoize from comment #38)
> Do we need a list of other tasks that need taking care as part of pushing
> this.. i.e. we'll need to update the sandboxes and ktd to run the new yarn
> commands (or perhaps point them at the new build_resources.PL script
> alternatively).

This is for https://gitlab.com/koha-community/koha-misc4dev/-/issues/60
I have a patch almost done, but I need a DB rev number to finish it (yes, that's not nice, we will depend on a DB rev for something not DB related...)
Comment 40 Martin Renvoize 2023-02-01 12:52:33 UTC
OK.. been testing running through the make process and this is looking great so far.

Not a blocker, but do we need both compiled css/js and source scss/vue files in blib?  I can't see a need for them and can't see a problem having them there.. but wondered if we wanted to clean that up whilst we're in here?
Comment 41 Martin Renvoize 2023-02-01 16:58:23 UTC
I tried to do an old-fashioned dev install to test this.. but alas, I failed miserably.

I had to sort out all sorts of dependencies to get the 'perl Makefile.PL' step happy.. then upon running 'make' it complained about yarn not being found.. so I installed yarn but then it complained with

yarn: error: no such option: --frozen-lockfile

So I tried replacing --frozen-lockfile with --immutable and that complained too.

In short.. I can run `build-resources.PL` perfectly from inside ktd.. though of course in that case we don't need the `yarn install` step at all as we have our our node_modules all there and waiting for us.. but trying to run it outside of ktd we end up stuck in dependancies hell.

I'm not sure where that leaves us.. we're not really actually expecting people to be running dev installs now.. and we don't really document it anywhere anymore as far as I can see on googling..  So maybe this is OK as I believe this should all work for packaging as it stands.

We need Mason to weight in really.
Comment 42 David Cook 2023-02-01 22:14:45 UTC
(In reply to Martin Renvoize from comment #40)
> Not a blocker, but do we need both compiled css/js and source scss/vue files
> in blib?  I can't see a need for them and can't see a problem having them
> there.. but wondered if we wanted to clean that up whilst we're in here?

Yeah, that thought crossed my mind as well. I don't know enough about scss/vue to know whether we should be putting them in a different source/build folder all together, or if we should update the Makefile.PL to just exclude them from being included. 

I think certainly a different ticket in any case though. But one worth thinking about just from a maintenance perspective I think.
Comment 43 David Cook 2023-02-01 22:50:13 UTC
(In reply to Martin Renvoize from comment #41)
> I had to sort out all sorts of dependencies to get the 'perl Makefile.PL'
> step happy.. then upon running 'make' it complained about yarn not being
> found.. so I installed yarn but then it complained with
> yarn: error: no such option: --frozen-lockfile
> 
> So I tried replacing --frozen-lockfile with --immutable and that complained
> too.

Which kind of dependencies did you need to sort out? Was it stuff like "build-essential gcc gettext" or something else? 

Which OS and OS version were you using? Was this in or outside of Docker?

Which version of yarn were you using? Was it Yarn v1, v2, or v3? What was the --immutable complaint? 

It looks like Debian/Ubuntu provide Yarn 1.x, and npm looks like it uses 1.x (although it provides 2.x too). 

> In short.. I can run `build-resources.PL` perfectly from inside ktd.. though
> of course in that case we don't need the `yarn install` step at all as we
> have our our node_modules all there and waiting for us.. but trying to run
> it outside of ktd we end up stuck in dependancies hell.

I needed the `yarn install` in my ktd as I didn't have node_modules already there. After a bit of reading, it sounds like running some type of `yarn install` invocation is a common step (although I suppose we could debate about when to run it).

I used to run Koha in OpenSuse and getting the dependencies right was always a pain, which is why we switched over to Debian/Ubuntu (and ktd for dev/test). 
 
> I'm not sure where that leaves us.. we're not really actually expecting
> people to be running dev installs now.. and we don't really document it
> anywhere anymore as far as I can see on googling..  So maybe this is OK as I
> believe this should all work for packaging as it stands.
> 
> We need Mason to weight in really.

Yeah, it would be good to hear from Mason. Perhaps I should have a go with my local package building as well and see if I bump into any gotchas...

Both single and dev installs are a bit fraught because of dependencies (which is a big reason I stopped doing them heh). Overall, I don't think the community directly supports anything except Debian package installs on Debian/Ubuntu, although we could always document better.
Comment 44 David Cook 2023-02-01 22:52:52 UTC
At the end of the day, I suppose the most important thing is that the Debian packages are built correctly for consumers of Koha, and then the second most important thing is that developers/testers are able to generate Koha installs without too much friction. 

__Packages__
If Mason can look into how this will work with the existing community package build workflow, that would be useful.

I need to do some other work right now... but I'll make a note to look at building a package using these patches. 

__Dev/Test__
Maybe we should have a crack at testing this process. I was interested by Jonathan's comment about using "watch" for dev/test.
Comment 45 David Cook 2023-02-02 05:10:30 UTC
So I'm looking at building a package with these patches, and I notice that the "yarn" in ktd is provided by this source list:

deb https://dl.yarnpkg.com/debian/ stable main
Comment 46 David Cook 2023-02-02 05:13:28 UTC
(In reply to David Cook from comment #45)
> So I'm looking at building a package with these patches, and I notice that
> the "yarn" in ktd is provided by this source list:
> 
> deb https://dl.yarnpkg.com/debian/ stable main

After I set up the repo:

sudo apt-get install yarn
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
The following additional packages will be installed:
  libc-ares2 libnode72 libuv1 nodejs nodejs-doc
Suggested packages:
  npm
The following NEW packages will be installed:
  libc-ares2 libnode72 libuv1 nodejs nodejs-doc yarn
0 upgraded, 6 newly installed, 0 to remove and 0 not upgraded.
Need to get 12.2 MB of archives.
After this operation, 54.1 MB of additional disk space will be used.
Do you want to continue? [Y/n] y
Comment 47 David Cook 2023-02-02 05:24:04 UTC Comment hidden (obsolete)
Comment 48 David Cook 2023-02-02 05:33:56 UTC
Everything was looking good in my build until I hit the issue where debian/control is still incorrect in the master branch...

xsltproc --output /home/koha/build/koha-22.12+test.bb175389/debian/tmp/debian/tmp_docbook/ \
    /usr/share/xml/docbook/stylesheet/docbook-xsl-ns/manpages/docbook.xsl \
    debian/docs/*.xml
warning: failed to load external entity "/usr/share/xml/docbook/stylesheet/docbook-xsl-ns/manpages/docbook.xsl"
cannot parse /usr/share/xml/docbook/stylesheet/docbook-xsl-ns/manpages/docbook.xsl
make[1]: *** [debian/rules:39: override_dh_auto_install] Error 4
make[1]: Leaving directory '/home/koha/build/koha-22.12+test.bb175389'
make: *** [debian/rules:9: binary] Error 2
dpkg-buildpackage: error: fakeroot debian/rules binary subprocess returned exit status 2
debuild: fatal error at line 1182:
dpkg-buildpackage -us -uc -ui failed

--

If the debian/control were correct, then I think the build would've succeeded. For example:

Installing /home/koha/build/koha-22.12+test.bb175389/debian/tmp/usr/share/koha/intranet/htdocs/intranet-tmpl/prog/js/vue/dist/main.js.LICENSE.txt
Installing /home/koha/build/koha-22.12+test.bb175389/debian/tmp/usr/share/koha/intranet/htdocs/intranet-tmpl/prog/js/vue/dist/main.js

Installing /home/koha/build/koha-22.12+test.bb175389/debian/tmp/usr/share/koha/opac/htdocs/opac-tmpl/bootstrap/css/opac.css

--

So... as long as the yarn is version 1.22.19... it's all looking good to me. 

I was building on a Debian Bullseye Docker image using https://gitlab.com/minusdavid/koha-deb-builder-docker
Comment 49 Martin Renvoize 2023-02-02 12:31:09 UTC
So I was installing on a fresh Ubuntu.. and trying initially to use ubuntu apt repos.. Yarn came in at a newer version that Koha expected.. that was the main stumbling block really.

I've since managed to get it all working.. thanks for the tip about that yarn repo.

We don't document a dev install anywhere now so it's become more difficult and I've become very rusty ;).

My feeling at this point is that we should go with this as is now :).

You can add my SO lines to the remote branch Jonathan.. I'm happy I got it all working for a standard install and a dev type install.. and it sounds like David was pretty happy with building a package, all be it he had expected issues due to upstream having a bad debian.control file at the minute (I fixed that in 22.11.x branch already.. not sure if he's got it building and applying to master regularly yet?)
Comment 50 Jonathan Druart 2023-02-02 13:50:09 UTC
(In reply to Martin Renvoize from comment #49)
> You can add my SO lines to the remote branch Jonathan

Done, thanks!
Comment 51 Katrin Fischer 2023-02-02 21:46:05 UTC
When everything is set, could you add a little write-up to the wiki?
Comment 52 David Cook 2023-02-02 22:32:42 UTC
(In reply to Martin Renvoize from comment #49)
> You can add my SO lines to the remote branch Jonathan.. I'm happy I got it
> all working for a standard install and a dev type install.. and it sounds
> like David was pretty happy with building a package, all be it he had
> expected issues due to upstream having a bad debian.control file at the
> minute (I fixed that in 22.11.x branch already.. not sure if he's got it
> building and applying to master regularly yet?)

Yeah I'm pretty happy :)
Comment 53 Jonathan Druart 2023-02-08 08:28:38 UTC
(In reply to David Cook from comment #52)
> (In reply to Martin Renvoize from comment #49)
> > You can add my SO lines to the remote branch Jonathan.. I'm happy I got it
> > all working for a standard install and a dev type install.. and it sounds
> > like David was pretty happy with building a package, all be it he had
> > expected issues due to upstream having a bad debian.control file at the
> > minute (I fixed that in 22.11.x branch already.. not sure if he's got it
> > building and applying to master regularly yet?)
> 
> Yeah I'm pretty happy :)

PQA then?
Comment 54 Jonathan Druart 2023-02-08 11:30:30 UTC
There is a merge request in misc4dev to support both build_js and build, see https://gitlab.com/koha-community/koha-misc4dev

It must be merged before this is pushed to master.
Comment 55 David Cook 2023-02-08 22:28:50 UTC
I wonder if Mason should install "yarn" on the package build machine(s) before this is pushed too, so that there aren't any delays building packages going forward?
Comment 56 Mason James 2023-02-09 09:50:08 UTC
(In reply to David Cook from comment #55)
> I wonder if Mason should install "yarn" on the package build machine(s)
> before this is pushed too, so that there aren't any delays building packages
> going forward?

i've added yarn (1.22.19-1) to the pkg build system
Comment 57 Martin Renvoize 2023-02-09 15:31:39 UTC
I've reviewed the joined misc4dev merge request too now.. I reckon we're PQA here so marking as such.
Comment 58 Tomás Cohen Arazi 2023-02-10 14:08:28 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 59 Martin Renvoize 2023-02-14 12:34:11 UTC
We decided not to backport this architectural enhancement.
Comment 60 Marcel de Rooy 2023-02-16 09:40:46 UTC
Any reason why the build-resouces.PL script includes a 1; at the end ?
Why the uppercase PL btw?
Comment 61 Jonathan Druart 2023-02-16 10:06:58 UTC
(In reply to Marcel de Rooy from comment #60)
> Any reason why the build-resouces.PL script includes a 1; at the end ?

Maybe not needed, but should not hurt.

> Why the uppercase PL btw?

https://metacpan.org/pod/ExtUtils::MakeMaker#How-To-Write-A-Makefile.PL
Comment 62 Martin Renvoize 2023-03-10 12:30:39 UTC
I decided to backport this one now.. it will aid in future maintenance, as it's already proved to cause a few minor issues with erm bugfix backports we've wanted to keep.

It's also proved itself nicely in master now so I feel it's a safe bet.

Backported for release in 22.11.04
Comment 63 Lucas Gass 2023-03-14 20:22:12 UTC
Won't backport to 22.05.x, unless requested.
Comment 64 David Cook 2023-03-30 22:47:17 UTC
(In reply to Martin Renvoize from comment #62)
> I decided to backport this one now.. it will aid in future maintenance, as
> it's already proved to cause a few minor issues with erm bugfix backports
> we've wanted to keep.
> 
> It's also proved itself nicely in master now so I feel it's a safe bet.
> 
> Backported for release in 22.11.04

Will know more soon! I'm working on a customized 22.11.03 and planning to merge in 22.11.04 when it comes out.
Comment 65 Aleisha Amohia 2023-04-26 05:04:16 UTC
Hi all,

This has caused some problems for our libraries upgrading to 22.11.04.

For example,

- the calendar icon that shows in the flatpickr text input fields does not show
- making return claims on the checkouts page shows the 'loading' icon forever, have to refresh the page to show the claim

Is there something we need to additionally do or install?

Thanks
Aleisha
Comment 66 David Cook 2023-04-26 05:39:54 UTC
(In reply to David Cook from comment #64)
> Will know more soon! I'm working on a customized 22.11.03 and planning to
> merge in 22.11.04 when it comes out.

(In reply to Aleisha Amohia from comment #65)
> This has caused some problems for our libraries upgrading to 22.11.04.

(In reply to Aleisha Amohia from comment #65)
> Is there something we need to additionally do or install?

I recently decided to pin at 22.11.03 for the time being, so I'm not sure what the problem might be.

I could probably look at this later this week if no one else does.
Comment 67 Jonathan Druart 2023-04-26 07:34:36 UTC
(In reply to Martin Renvoize from comment #62)
> Backported for release in 22.11.04

Did you ask Mason to adjust the pkg workflow to embed the css and js files?
Comment 68 Martin Renvoize 2023-04-26 10:21:52 UTC
(In reply to Jonathan Druart from comment #67)
> (In reply to Martin Renvoize from comment #62)
> > Backported for release in 22.11.04
> 
> Did you ask Mason to adjust the pkg workflow to embed the css and js files?

Community packages are all working as expected here.. I presume your building your own Aliesha.. in which case the core thing is to ensure you run the build-resources.PL script as part of your package building process.. that will compile the CSS and JS resources for you.
Comment 69 Aleisha Amohia 2023-04-26 23:02:39 UTC
(In reply to Martin Renvoize from comment #68)
> (In reply to Jonathan Druart from comment #67)
> > (In reply to Martin Renvoize from comment #62)
> > > Backported for release in 22.11.04
> > 
> > Did you ask Mason to adjust the pkg workflow to embed the css and js files?
> 
> Community packages are all working as expected here.. I presume your
> building your own Aliesha.. in which case the core thing is to ensure you
> run the build-resources.PL script as part of your package building process..
> that will compile the CSS and JS resources for you.

Hey, nope, this problem is occurring for our libraries installing stock Koha from the community tag, so not building the packages ourselves

$ apt-cache policy koha-common
koha-common:
  Installed: 22.11.04-1
  Candidate: 22.11.04-1
  Version table:
 *** 22.11.04-1 500
        500 http://debian.koha-community.org/koha 22.11/main amd64 Packages
        100 /var/lib/dpkg/status

How can I run the build-resources.PL script please? Couldn't see any documentation on the Community wiki
Comment 70 David Cook 2023-04-26 23:03:42 UTC
(In reply to Martin Renvoize from comment #68)
> Community packages are all working as expected here.. I presume your
> building your own Aliesha.. in which case the core thing is to ensure you
> run the build-resources.PL script as part of your package building process..
> that will compile the CSS and JS resources for you.

The Makefile should be running build-resources.PL as per https://gitlab.com/joubu/Koha/-/commit/b64db106e0db41da06fbf72f1674b2ed43b3f76d

If the overall process was failing, I think Aleisha would have different symptoms. The issue she's having might be unrelated to this change but won't know without more information (provided either by Aleisha or someone else investigating).
Comment 71 David Cook 2023-04-26 23:05:03 UTC
(In reply to David Cook from comment #70)
> If the overall process was failing, I think Aleisha would have different
> symptoms. The issue she's having might be unrelated to this change but won't
> know without more information (provided either by Aleisha or someone else
> investigating).

Actually, on that note, I think Aleisha should probably open a separate bug report for the issues she's facing and do a "See Also" to this one in case it's related.
Comment 72 Aleisha Amohia 2023-04-26 23:06:48 UTC
(In reply to David Cook from comment #71)
> (In reply to David Cook from comment #70)
> > If the overall process was failing, I think Aleisha would have different
> > symptoms. The issue she's having might be unrelated to this change but won't
> > know without more information (provided either by Aleisha or someone else
> > investigating).
> 
> Actually, on that note, I think Aleisha should probably open a separate bug
> report for the issues she's facing and do a "See Also" to this one in case
> it's related.

Will do. I did a git bisect to narrow down the commit where we stopped seeing things we expected to see but there could be something else at play in the packaging process or other.
Comment 73 Tomás Cohen Arazi 2023-04-26 23:08:37 UTC
As I posted on the koha-devel list:

"
Kyle and I have been working on this:

https://gitlab.com/koha-community/koha-dpkg-docker#usage

which is a simple way to build packages, and considers that required step.

Hope it helps.
"
Comment 74 Aleisha Amohia 2023-04-26 23:13:16 UTC
Thank you Tomás but as I said, we aren't building our own packages. It is community packages that we get this problem
Comment 75 David Cook 2023-04-26 23:13:59 UTC
(In reply to Tomás Cohen Arazi from comment #73)
> As I posted on the koha-devel list:
> 
> "
> Kyle and I have been working on this:
> 
> https://gitlab.com/koha-community/koha-dpkg-docker#usage
> 
> which is a simple way to build packages, and considers that required step.
> 
> Hope it helps.
> "

Could we get more details on why you did it that way? It should be unnecessary...
Comment 76 David Cook 2023-04-26 23:15:00 UTC
(In reply to Aleisha Amohia from comment #74)
> Thank you Tomás but as I said, we aren't building our own packages. It is
> community packages that we get this problem

It looks like there are some demo boxes on 22.11 so I'm happy to help Aleisha out with this one (on a separate bug report).
Comment 77 Aleisha Amohia 2023-04-26 23:22:45 UTC
(In reply to David Cook from comment #76)
> (In reply to Aleisha Amohia from comment #74)
> > Thank you Tomás but as I said, we aren't building our own packages. It is
> > community packages that we get this problem
> 
> It looks like there are some demo boxes on 22.11 so I'm happy to help
> Aleisha out with this one (on a separate bug report).

Thank you. I've reported Bug 33621
Comment 78 David Cook 2023-04-27 00:03:56 UTC
(In reply to Aleisha Amohia from comment #77)
> Thank you. I've reported Bug 33621

Thanks, Aleisha.

It looks like one issue is probably due to bug 32978 and the other looks like a coding bug.
Comment 79 Caroline Cyr La Rose 2023-05-12 13:22:14 UTC
I don't think there is anything to document here. Closing.
Comment 80 David Cook 2024-02-05 05:54:22 UTC
I was just looking at the Debian packaging logs and wondering why build-resources.PL gets run twice, but it's because it's run for "make" and "make install". 

And that's because in Makefile.PL there is the following line:

install :: all install_koha set_koha_ownership set_koha_permissions warn_koha_env_vars

I suppose if it really bothered us we could do something about it, but it doesn't really seem like a big issue. Wastes a bit of time, but better to do it too much than not enough...