Patch development usability has got worse

[mcclure]

In late 2020 I checked out version 2918c48 of this repo on my Mac, put some patches in PatchSource, and started. It worked great and I was a happy developer.

This week I checked out 38448cc on my Windows machine (same as 0d6c6f2 for purposes of the following bugs; somehow I accidentally checked out the feature/dsp-updates branch at first instead of master or develop) and started trying to build my own patches. I eventually got everything working, but I ran into a series of problems. Some were just annoying but some lost me a great deal of time trying to debug. Overall the experience of developing patches locally with OwlProgram+FirmwareSender is definitely worse than it used to be. The Mac and Windows machines are running the same arm-none-eabi-gcc (gcc version 9.2.1 20191025 (release) [ARM/arm-9-branch revision 277599]) so this is due to changes in the repo.

What I encountered:

1. Problem 1: make libs required now

   When I run make patch on a new checkout, it fails saying that two .a files are not found. This is documented in the README but it is still a little nonobvious.

   Suggestion: make libs should be a dependency of make patch, so that the libs are built automatically when you build the patch.

   If that is hard, an easier suggestion would be to detect specifically that the .a files are missing at the start of make patch and print a clear error message saying something like "x not found: must run make libs. See README".

2. Problem 2: make libs fails without emscripten

   When I run make libs it runs for a long time then fails saying "emcc" is not found. This also is documented in the README but again it is nonobvious and the error message is nonobvious (I had to google what "emcc" was). The idea of an "expected failure" on a make command is also very problematic because it means you cannot use normal UNIX idioms with make libs such as make libs && make patch.

   Suggestion: There should be separate make libs and make weblibs targets. make patch should depend on make libs but not make weblibs, and make libs should work without emscripten. (Or if you like, something like make patchlibs for the .a files, make weblibs for the emscripten stuff, and make libs that triggers both of them.)

3. Problem 3: Badly misleading errors when incorrect PLATFORM specified*

   When I started making patches this week I was working off my notes from 2020. So the way I built was: make patch TOOLROOT= PATCHNAME=Silence PLATFORM=Magus. However, the platform names have changed, it is now OWL2 not Magus. This would be okay except that if you run with a non-empty but unrecognized PLATFORM, it fails with a highly misleading error:

    /usr/lib/gcc/arm-none-eabi/9.2.1/../../../arm-none-eabi/bin/ld: cannot open linker script file -fsingle-precision-constant: No such file or directory
   

   I wasted a lot of time trying to debug this, I assumed at first that a GCC-only argument was being passed to the linker... in fact the problem was the line LDFLAGS += -T$(LDSCRIPT) $(ARCH_FLAGS), if the platform is unrecognized LDSCRIPT is empty.

   Suggestion: PR Print error message when platform not recognized #111

4. Problem 4: make clean required every time

   In my testing with 38448cc and 0d6c6f2 , if I build and run my ScreenSaver patch, then run (cd .. && make patch TOOLROOT= PATCHNAME=ScreenSaver PLATFORM=OWL2 && ../FirmwareSender/Builds/VisualStudio2015/Win32/Release/ConsoleApp/FirmwareSender.exe -in Build/patch.bin -out OWL-MAGUS -run), then make a change to ScreenSaver, then repeat the build and install, nothing changes. The make patch has no effect. I have to make clean every time.

   The README does say: "Make sure to do a make clean before compiling a new patch, or add clean to your make target (e.g. make clean patch), otherwise the previous patch name will be retained." This is how things used to work, but now contrary to the README it appears clean is required even when building the same patch over.

   I lost a pretty long time on this last night thinking I was using the new screen features wrong when in fact my changes were simply not being deployed to the device.

   Suggestion: Obviously it would be nice if make patch understood changes and make clean were not required. Having to make clean every time you switch patches is annoying and easy to forget, but tolerable. Having to make clean every time is livable, but with large patches this will start to get a bit slow..!

   If this is hard to fix, then either make patch should simply trigger make clean automatically, or at least the README should be changed to warn make clean is required every time.

I think all of these are "papercuts" except (3), so I made a PR for that one.

[pingdynasty]

Good, thoughtful feedback, ty @mcclure.

A lot of this has to do with flaws in how dependencies are managed in the Makefiles.

1 and 2: The libs are built separately to speed up the build time when just compiling a patch, and not making changes to the libraries. Which is most of the time. But then make clean removes all object files, including those used to make the libs, which means that if the patch compilation target was dependent on the libs as you suggest then they would be rebuilt every time you make clean. I really don't like the way it currently works, especially how DaisySP and JS libs are integrated even though they're mostly not used. It's on my todo list to fix this but I'm not sure how.

3. thank you for the fix, merged!

4. this is a long standing problem which should be fixable, but doing it in a way that works for all patch types (gen, pd, c++, faust, soul...) is tricky.

it appears clean is required even when building the same patch over

I didn't think that was the case, I will investigate.