vedderb / vesc_pkg

Official VESC Packages
GNU General Public License v3.0
34 stars 42 forks source link

Move the build system to Makefiles, cleanups #21

Closed lukash closed 1 year ago

lukash commented 1 year ago

Attempting to clean up the build system. I've removed the build_res script altogether, as I'm not a fan of build system script wrappers, but if required, it can be added with the contents of:

make clean
make

I've added this to the README. Likewise, I've done away with the VT=/usr/local/bin/vesc_tool, it now defaults to vesc_tool, it's better without the hardcoded part. README says how to override that, can be tweaked too.

I've removed all vescpkgs from the git and now generate them using the full vesc_tool spec string. Honestly re-using the old ones for some data is very non-standard and not a good practice.

Some changes are specific to the float package, tagging @NicoAleman, @surfdado and @Mitchlol, please check it out. I've cleaned up the way the dynamic README contents are generated. Then there's a commit that auto-generates the confxml.{c,h} instead of having to generate them manually. The two tiny commits changing the contents of the generated package README are obviously subjective :slightly_smiling_face:

surfdado commented 1 year ago

How do I get past this? Ubuntu 20.04

make -C balance 
make[1]: Entering directory '/home/dmista/fun/vesc_pkg/balance'
make -C balance
make[2]: Entering directory '/home/dmista/fun/vesc_pkg/balance/balance'
make[2]: Nothing to be done for 'balance.c'.
make[2]: Leaving directory '/home/dmista/fun/vesc_pkg/balance/balance'
vesc_tool --buildPkg "balance.vescpkg:balance.lisp:ui.qml:0:README.md:Balance"
qt.qpa.plugin: Could not find the Qt platform plugin "offscreen" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: xcb.

Aborted (core dumped)

Also, what exactly is the benefit of switching to using make at the top level?

surfdado commented 1 year ago

Also, something doesn't feel right if a makefile requires a Qt frontend to work. Building a package used to have a super low barrier to entry... All you needed was the ARM Gnu toolchain and run the build_res script...

Mitchlol commented 1 year ago

I cant get the qt front end to work from the command line so this would totally break builds for me :'(

lukash commented 1 year ago

How do I get past this? Ubuntu 20.04

make -C balance 
make[1]: Entering directory '/home/dmista/fun/vesc_pkg/balance'
make -C balance
make[2]: Entering directory '/home/dmista/fun/vesc_pkg/balance/balance'
make[2]: Nothing to be done for 'balance.c'.
make[2]: Leaving directory '/home/dmista/fun/vesc_pkg/balance/balance'
vesc_tool --buildPkg "balance.vescpkg:balance.lisp:ui.qml:0:README.md:Balance"
qt.qpa.plugin: Could not find the Qt platform plugin "offscreen" in ""
This application failed to start because no Qt platform plugin could be initialized. Reinstalling the application may fix this problem.

Available platform plugins are: xcb.

Aborted (core dumped)

I have made no changes whatsoever to this, the commands being run to do the build are the same as before. The reason here is probably the vesc_tool you have in PATH is different from the one that was being run before, that is /usr/local/bin/vesc_tool, and seems quite broken. (try make VESC_TOOL=/usr/local/bin/vesc_tool)

Also, what exactly is the benefit of switching to using make at the top level?

It's cleaner. Instead of a single script that mashes everything together, the targets are now separate. You can build each package independently and the "recipe" to build them resides in the package directory itself, instead again being all together in a single bash script. It also provides a way to uniformly propagate e.g. the VESC_TOOL definition.

lukash commented 1 year ago

@surfdado did you get back to looking into the vesc_tool issue?

surfdado commented 1 year ago

@surfdado did you get back to looking into the vesc_tool issue?

I got a new VESC laptop now, I will try again today

lukash commented 1 year ago

@surfdado did you get back to looking into the vesc_tool issue?

I got a new VESC laptop now, I will try again today

I see there's a new version, I'll rebase the PR later. Your commit: 2487ac6 Float Package: Inject version number into ui.qml file can be done better, in a similar manner to appending the version to README.md (and then you don't need the git checkout and comment it out etc.). I'll add that in the rebase.

I've also noticed the confparser.h file is also generated by vesc_tool, I'll adjust for that too.

And finally, the vesc_pkg-float repo, having a different history, will likely need this cherry-picked. I can do that too and make another PR in that repo if you want.

surfdado commented 1 year ago

So make does work for me now. Sorry for making your rebase. But don't worry about our float vesc_pkg repo - I will pull in your change once you've rebased your version on the official repo.

lukash commented 1 year ago

Rebased and amended. ui.qml is now generated from ui.qml.in, which contains a template placeholder for the version. Future ideas:

Sorry for making your rebase.

No worries, rebase is a way of life :sunglasses:

surfdado commented 1 year ago

I can confirm this builds for me, but I have mainly one concern: removing confparser.h is problematic because as a 3rd party app developer I've come to rely on the SIGNATURE in there to check whether any commit has broken compatibility with existing parsers. Just like I can do a git diff to check if app-config or motor config have changed in the firmware, I like to be able to do the same in the package too.

So being able to track the SIGNATURE in the git history I think should be preserved.

lukash commented 1 year ago

Couldn't you look for changes in the settings.xml itself?

Just to be entirely sure, what signatures are you comparing, one from a .vescpkg and the one in git?

Worst case I can put the confparser.h back into git, but it's not entirely foolproof, because one can edit settings.xml, not re-generate the signature and commit. Then you go to build, and the run of the vesc_tool will update the signature in the working tree, and build with a different signature. Hopefully someone notices and realizes it's wrong. So it'd be much better if it wasn't in git and nothing relied on the signature being there.

surfdado commented 1 year ago

It's not possible to easily see this in settings.xml, as many changes don't end up changing the signature, which is like the checksum of the serialized configuration data. Example for the firmware: image

Here I can see that my branch has no differences to 6.0 - whereas I can see that in 6.05 the motor configuration has changed (app configuration has not) - so any 3rd party app will need a new parser to be able to read/write 6.05 configurations.

surfdado commented 1 year ago

I fully support keeping generated files out of git, but in this case I need this generated signature, for example if I want to quickly find which commit(s) changed the signature.

Now technically I don't even care whether configparser.c/h are tracked in git, as long as we have some file that includes that SIGNATURE so that it will show up in a git diff when comparing branches

lukash commented 1 year ago

I see. Well, I'm still of the opinion this shouldn't be stored in git, for the sole reason that the signature stored in git can be simply wrong and not reflect the data in settings.xml (it's duplicate information). The signature can be stale if someone changes only settings.xml and doesn't re-generate (yes, I've already mentioned this :grin:).

Hence, I think you should do this instead of your simple diff grep:

git checkout origin/master && make float && grep SIGNATURE float/float/conf/confparser.h

And compare that to the signature you have, which isn't too much harder and you will have 100% certainty everything is right.

If you still insist on having the signature in git, I'll be forced to return the confparser.h into the git tree (there's no reason to store the signature elsewhere, that' just make things more complex and won't be any better concept-wise).

surfdado commented 1 year ago

Obviously the git command you're suggesting isn't practical at all, especially if you have uncommitted changes or want to quickly check multiple branches/commits

I'd say let's just put confparser.h back into git

vedderb commented 1 year ago

If it helps I can add a cli-argument to VESC Tool that prints the signature of an XML.

surfdado commented 1 year ago

that may be a useful feature in the vesc tool, but it doesn't change the underlying problem when removing confparser.h from git - today when using "git diff|grep SIGNATURE" I can compare any commit I want without doing a git checkout

lukash commented 1 year ago

I'm ready to add that file back to git, just need to do it...

But I wanted to mention that now, with this build system, your git directory will always be clean (of temporary files you might want to keep, etc.), barring any uncommited changes. You can commit your changes anytime, in a WIP commit, or just finish the thing you're doing, and then you can easily switch branch and do the check. Then, a quick make clean && make will get you back to having a package built in a couple of seconds, clean and reliable.

Or another way, having a second clone for the purpose of the check would also do the job. I consider this a bit of a "false convenience", but again... up to you :slightly_smiling_face:

vedderb commented 1 year ago

Generally I don't think keeping the confparser files in git is a problem if it helps. I do like the other changes to the build system in this PR. If @surfdado, @NicoAleman and @Mitchlol are fine with the changes, maybe after adding confparser back, I can merge them. I'm also ok with leaving it as it is, the important thing is that it works well and is easy for the contributors.

surfdado commented 1 year ago

Excellent - I'm happy with the proposed changes if we keep the confparser files in for now.

lukash commented 1 year ago

I've returned the file. I kept it as a separate commit, in case someone wanted to ultimately remove it, it'd be a bit easier by reverting the commit.

lukash commented 1 year ago

@vedderb shall we merge unless there's more outstanding issues? @NicoAleman @Mitchlol one more ping to provide feedback if you have any. Thanks!

surfdado commented 1 year ago

@vedderb shall we merge unless there's more outstanding issues? @NicoAleman @Mitchlol one more ping to provide feedback if you have any. Thanks!

Nico is fine with it and I think Mitch already said earlier that he's okay with it