nasa / fprime

F´ - A flight software and embedded systems framework
https://fprime.jpl.nasa.gov
Apache License 2.0
10.09k stars 1.31k forks source link

fprime-util generate should not fail if run twice #2869

Open weggert2 opened 1 month ago

weggert2 commented 1 month ago
F´ Version 3.4.3
Affected Component N/A

Problem Description

fprime-util generate fails if it has already been run, even if the first run was successful.

A typical cmake workflow is cmake .. && make. The parallel for F' would be fprime-util generate && fprime-util build, which fails after the first generate. Understood that fprime-util build probably re-runs generate if it needs to, and you only need to run generate once, but from a 'use tools like you're used to' perspective, fprime-util generate should be indempotent and not fail if it has nothing to do.

Context / Environment

Execute fprime-util version-check and share the output.

$ fprime-util version-check
Operating System: Linux
CPU Architecture: x86_64
Platform: Linux-6.8.0-40-generic-x86_64-with-glibc2.35
Python version: 3.10.12
CMake version: 3.22.1
Pip version: 24.2
Pip packages:
    fprime-tools==3.4.4
    fprime-gds==3.4.3
    fprime-fpp-*==2.1.0a3

How to Reproduce

Run fprime-util generate twice

Expected Behavior

Similar to cmake, the second time should either print 'already generated', or some other small message and return success. This is useful for scripting 'generate' and 'build' tasks which rely on the error code.

thomas-bc commented 1 month ago

This is a good point. It would also remove the need to purge the build cache if generation hasn't been successful in some instances. What do we think would be the best behavior:

zimri-leisher commented 1 month ago

I think we should mirror what CMake does. As far as I know, it doesn't ask for any confirmation if you re run cmake. I also don't think it should force a regenerate, because I think build already does this to some extent (although I am unclear as to when)

thomas-bc commented 1 month ago

Agreed!

If anyone would like to contribute for this, changes likely should be around this function or wherever it gets invoked by fprime-util generate https://github.com/nasa/fprime-tools/blob/b72544568269010abf2197d9b0dec10bce40e71c/src/fprime/fbuild/builder.py#L371

weggert2 commented 1 month ago

I'm happy to contribute here, but it would help to have some context on why this behavior exists, since it's called out directly in the docs:

def invent(self, platform: str = None, build_dir: Path = None):
        """Invents a build path from a given platform

        Sets this build up as a new build that would be used as as part of a generate step.
        This directory must not already exist.

What pitfalls are there to just not raising the error (i.e. removing the following snippet)

        if self.build_dir.exists():
            msg = f"{self.build_dir} already exists."
            raise InvalidBuildCacheException(msg)

reference: https://github.com/nasa/fprime-tools/blob/b72544568269010abf2197d9b0dec10bce40e71c/src/fprime/fbuild/builder.py#L92

thomas-bc commented 1 month ago

It might be that this snippet can just be removed. But I don't know, there may be something else under.

Here's how you can test it out yourself with a local install: https://github.com/nasa/fprime-tools?tab=readme-ov-file#developer-installation

weggert2 commented 1 month ago

Alright, PR open! Thanks for all the quick responses.

thomas-bc commented 1 month ago

cc @csmith608