Release v3.2.0 #1973

thomas-bc · 2023-04-20T00:23:55Z

Release notes

F´ v.3.2.0 comes with FPP, Tooling and GDS improvements. The underlying typing system has been improved to allow logical types.

Breaking Changes

Authors of CMake toolchains should now supply a PlatformTypes.h header according to the Numerical Type design. Linux example provided here: cmake/platform/types/PlatformTypes.h.
LinuxSerialDriver has been renamed LinuxUartDriver and has been updated to support the ByteStreamDriver model. Users will need to update their ports.
Users should consider adding ComQueue and ComAdapter to their downlink chain.
CAssert.hpp renamed CAssert.h

Major Changes

GDS now supports complex types, command arguments and parameters.
Sanitization has been added to Unit Tests.
FPP now directly generates data types and ports.
fprime-util new now supports generating new projects (as well as deployments and components). Users are recommended to use this new functionality moving forward.
Getting Started tutorial has been reworked and leverages the fprime-util new functionality.

New Contributors

[...]

Full Changelog: v3.1.1...v3.2.0

Replace Inspection with Unit test

…f just _SEP

* lestarch: adding numerical types (planned) design * lestarch: review comments

* Recursively check for type imports * Fix some Python formating * Make prm type checks for lax * More formatting

* Revising the getting started tutorial to cover getting started * Adding a generate step * Additional generate note * Clarifying the directory for running commands * Adding actual output to New Project * Updating HelloWorld.md with tool output * Toushing up Deployments.md * sp * Fixing spacing * Removing prereqs * Update Tutorial.md to move next link

* Fix tutorial link * Update README ! * Fixes * Add user guide to getting started * Resources * Review update * Peer review * Bump tooling versions * spelling

* Bump tooling and GDS to 3.2.0 * update FALLBACK_VERSION * Adding CMake User API to user guide

Drv/LinuxI2cDriver/LinuxI2cDriver.hpp

@@ -29,7 +29,7 @@

      //! Construct object LinuxI2cDriver
      //!
-      LinuxI2cDriverComponentImpl(const char *const compName);
+      LinuxI2cDriver(const char *const compName);


github-advanced-security

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

thomas-bc · 2023-04-20T00:40:35Z

For some reason the Spell Checking workflow chose to run on 0.20 when 0.21 is specified, which seems to be the reason it failed.... Investigating

thomas-bc · 2023-04-20T01:14:23Z

Apparently related to the usage of pull_request_target: https://github.com/nasa/fprime/blob/devel/.github/workflows/spelling.yml#L42
Which checks out the target of the PR instead of the merge commit

thomas-bc · 2023-04-20T01:52:34Z

@jsoref could weigh in on why you chose to use the pull_request_target event in the spelling workflows? I tried looking in check-spelling/check-spelling but couldn't find any mentions regarding this choice. From the documentation I found online (e.g. here), it seems like this can be a rather unsafe behavior - and I don't see why it would be necessary rather than just use the classic pull_request event. Did I miss something?
Thank you!

jsoref · 2023-04-20T10:04:28Z

(I'm writing this in the middle of the night)
Summary: In modern GitHub workflows, like this one, permissions are controlled by the permissions field. GitHub's initial workaround for needing permission to write was pull_request_target. Their latest workaround is step summary. But, there are quirks relating to how things are processed when a head branch changes a workflow and there are quirks relating to merge conflicts. And there are quirks for when the base branch workflow changes. (It's 🐢🐢 all the way down.)

Historical background:
In order for check-spelling to comment on a PR or push, check-spelling needed write permissions. For a pull request, that was be pull-requests: write. For a push, that was contents: write.

The initial GitHub workflows model didn't really offer permissions management. Instead, the default/maximal permissions varied based on whether a pull request was from the same repository or a fork. And to get permissions to write (=comment) for pull requests from forks you had to use pull_request_target. You can read about the headaches stemming from this initial workaround (and the negative security consequences) in: https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

Permissions came along much later and can be seen here:
https://securitylab.github.com/research/github-actions-building-blocks/

Historically, and mostly even today, forks tended not to run workflows. Which meant the only time most users could get feedback was when they created their PR instead of when they pushed to their fork.

Beyond that, the error handling for merge conflicts turned out to be confusing to users–they'd push and not understand why check-spelling didn't comment.

Anyway, changing to pull_request would not improve security because it's the write permissions (which are explicitly off for this workflow) that makes things insecure.

Modern GitHub has a GitHub step summary which check-spelling uses (especially here) instead of relying on commenting.
https://github.blog/2022-05-09-supercharging-github-actions-with-job-summaries/

For security, check-spelling's comment phase is a distinct job, with write permissions (and it wouldn't work for PRs from forks if the pull_request event was used). But this repository isn't using that job, and thus there shouldn't be a risk.

I'm pretty sure there's a different rough edge involving a PR from a fork that changes the workflow file (if you're using pull_request), because if the workflow change is dangerous, GitHub can't risk running that change.

The downside to most of these configurations is that changes to the workflow file result in rough edges. Here the problem is that the dictionaries that are being used aren't the same as the ones being expected. If your PR head is passing, as it is here, then you can safely ignore the problem and it'll go away when you merge. Or, if there are actually changes in the head branch that aren't in the PR head, you'll be able to clean them up after your merge.

In the future, I intend to move the dictionary configuration out of the workflow which would enable some of this problem to be avoided. But I haven't decided how I'd want it to work and haven't been able to reason through the transition – if there are dictionaries defined in the workflow and also the files, which wins? I have some code right now that I was testing out for some "simpler" migrations – just renaming action parameters – and that made such a mess that I'm not using it, it's just lying around in a branch. And if there's a difference in the engine version (which in turn can have different default dictionaries, as happened here), then what? – thinking about it now, I think if my config file defined the dictionary version, check-spelling could determine if it's newer than itself and perhaps run twice/be cautious about the config change.

Unfortunately, there's a GitHub bug (which I believe I've reported, likely repeatedly) where GitHub shows the wrong workflow file for pull_request_target as happened here. – Essentially GitHub developers want to forget (or have forgotten?) that pull_request_target exists.

You can try switching to pull_request. On average it'll be fine – some of my users use it. I'm pretty sure you'll hit rough edges elsewhere either for when there's a merge conflict (with a merge conflict, pull_request doesn't run but pull_request_target does – and check-spelling should give a reasonably friendly error message for this case) or when someone changes the workflow on the PR head or the base – roughly the circumstances you're in here anyway. There are some other fun quirks where the test merge commit ends up being in the wrong state for conflicts (I can't remember the details).

– I'm sorry that this is a long description and that it probably doesn't clear things up much.

timcanham

It seems that the ChangeLog.md has been neglected. Can we update that file or remove it if we aren't using it? I think we should keep it there and put in entries and release dates for the missing releases including the current one.