Cleanup and add a Makefile #161

alistair23 · 2020-03-10T00:42:08Z

After looking at #160 I think there are some things we can do to make it easier to get started with libtock-rs. One of the key things for me with all new projects is how hard is it to build and run this thing. I find the current bundle of scripts confusing and non obvious what I should do/run. This PR moves the linker files out of the main directory (so they don't clutter everything up) and then adds a Tock style Makefile which can be used to build the examples for the boards.

This PR also removes some unused files to again reduce clutter.

If this is merged the next step is to move the flash.sh actions into the Makefile and look at supporting building a single app (instead of all of the examples) and how to handle features.

Signed-off-by: Alistair Francis <[email protected]>

labbott

I like the boards cleanup but I'm not sure if the Makefile is better. We were just relying on cargo to build and now we have cargo + make to maintain. Is this something we could supplement with just documentation?

Makefile

torfmaster · 2020-03-10T17:35:09Z

Thank you for your contribution, I like the idea of moving the layout files. Can we please split out the discussion about the build system in general and the creation of a Makefile in particular to a separate issue? I created one for that topic: #162

alistair23 · 2020-03-10T17:47:07Z

The advantage with Makefiles is that it's easy for new people to understand. Someone can clone this repo and run make and know what to do. The current method of run some random scripts is extremely confusing. Documenting something that is confusing only helps so much as people don't read documentation and that effort could be better spent just making it not confusing.\

As for just using Cargo, I don't think Cargo is powerful enough or easy to use enough for our use case right now. It's also not common (and not used outside of Rust), so it's another learning curve.

On top of that, Tock and libtoc-c both use Make, so now we all use the same thing.

ppannuto · 2020-03-10T17:58:44Z

Quick note:

The advantage with Makefiles is that it's easy for new people to

understand. Someone can clone this repo and run make and know what to do. Was the exact impetus for doing this in the kernel. Phil argued strongly (and in retrospect correctly) that being able to clone a repo, type "make", and have something useful happen is very important, even if the Makefile is just a one-liner that invokes "cargo build" [obviously, the kernel make does more]. I tried briefly to introduce more complex build systems into libtock-c, but we walked that back. The reality is that one person investing complexity into a well-known build system (there are so many $'s in those Makefiles) is better than 1000 new people learning something complex to be able to build a project. The interface is important, and for better or worse, decades of software history have taught people "(1) Download software. (2) Type 'make'."

…

On Tue, Mar 10, 2020 at 10:47 AM Alistair Francis ***@***.***> wrote: The advantage with Makefiles is that it's easy for new people to understand. Someone can clone this repo and run make and know what to do. The current method of run some random scripts is extremely confusing. Documenting something that is confusing only helps so much as people don't read documentation and that effort could be better spent just making it not confusing.\ As for just using Cargo, I don't think Cargo is powerful enough or easy to use enough for our use case right now. It's also not common (and not used outside of Rust), so it's another learning curve. On top of that, Tock and libtoc-c both use Make, so now we all use the same thing. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#161?email_source=notifications&email_token=AACS3XRRDJXVBLC6VTEIVWTRGZ4KVA5CNFSM4LEUJSCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOMOPKQ#issuecomment-597223338>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACS3XUEQUZG5YO3SSFIJITRGZ4KVANCNFSM4LEUJSCA> .

alistair23 · 2020-03-10T18:05:44Z

Exactly! It's what everyone expects and our cargo commands are complex enough that we can't just say type cargo so we need something else around it.

In no way is this a move to get rid of Cargo. Replacing Cargo doesn't give you anything, this is just a simple wrapper to avoid people having to remember/discover the magic arguments to get their board working.

Woyten

I am not very convinced that make is a good tool. From my understanding, it was designed to provide build management and change detection to old programming languages. Something that has become completely obsolete with cargo.

From my point, it's okay to use make if people insist. But I would like to let you know that when I got in touch with Tock OS for the first time, my reaction was that I wanted to stop immediately being annoyed by the fact that I was forced to install make which I considered a tool from the stone age.

Thus, using make is not a guarantee that people will appreciate the toolchain as in my case it lead to the opposite effect.

rustfmt.toml

CHANGELOG.md

Woyten · 2020-03-10T18:55:03Z

tools/list_boards.sh

+#!/usr/bin/env bash
+
+# Find boards based on folders with Makefiles
+for b in $(find boards -maxdepth 4 | egrep 'layout'); do
+ b1=${b#boards/layout_}
+ b2=${b1%.ld}
+ echo $b2
+done


Could this be a part of the makefile?

It might be able to, but I think it might be cleaner being outside

bradjc · 2020-03-10T19:28:23Z

I agree that make is not good. But it is ubiquitous. Having cargo build work would be amazing, but I doubt it will ever be possible to have just cargo build actually work.

Having make do something useful is really nice.

alistair23 · 2020-03-10T21:21:12Z

I'm amazed you didn't have Make installed, it's so commonly used in open source projects.

Right now you can build the OpenTitan platform, but accidentally enable atomics via the target flag, which results in a binary that will crash. There is no way that is clear to users and is prone to errors.

I have updated the Makefile to support: flashing, features and debug builds. It's all self documenting (make will tell you what to do) and it's in the README. This should be much easier for anyone getting started.

Signed-off-by: Alistair Francis <[email protected]>

Woyten · 2020-03-10T22:22:19Z

I'm amazed you didn't have Make installed, it's so commonly used in open source projects.

It is commonly used in old programming languages that aren't shipped with build management. As I said, for me it's a tool from the stone age of computer science and I was not expecting to ever use it again, especially in the context of Rust which is shipped with a build management system. Keep in mind that, like me, there are a lot of people that never use it and don't have it installed since it's not required when working with more modern languages like Java, Phython, JavaScript or Rust.

Installing dependencies like Make was one of the hurdles when we did a workshop involving libtock-rs at our company. People tend to say that they don't want to pollute their system with Make only to play around with libtock-rs. It's just an old piece of software that is no longer installed on every system.

torfmaster · 2020-03-10T22:24:30Z

Can we move this discussion to #162 and merge the other changes in this PR in the meanwhile? It's not just adding a Makefile but adding just another build system to the existing one (I doubt that Makefiles will replace cargo, but if we were consequent, it should). I think we should have a structured discussion about the requirements we have for the build system.

Woyten · 2020-03-10T22:51:55Z

README.md

-Instead of specifying an environment variable each time you can permanently configure your platform by writing its name into a file named `platform`, e.g.
-
-```bash
-echo nrf52 > platform
-```


So, this feature is no longer supported? Then, it should be removed from the build.rs script. Or you might consider bringing it back?

What do you mean?

The build.rs checks whether the PLATFORM env variable is set and, if not, checks whether the file platform is present.

It will still do that, I haven't change build.rs. I just don't think this needs to be documented in the top level README anymore as it isn't really user exposed.

Makefile

Woyten · 2020-03-10T23:01:22Z

Could you adapt Contributing.md and consider removing doc/README.md?

alistair23 · 2020-03-11T01:11:37Z

I just pushed an update addressing all of your comments.

README.md

torfmaster · 2020-03-11T07:24:02Z

Having make do something useful is really nice.

I partially agree. But this PR commits libtock-rs to having a Makefile instead of a modern build system also in the CI. Moreover, we just wrap some functionality of Cargo in a Makefile (instead of just deleting Cargo.toml).

I see important points in the tock kernel build system not to use make:

tests are run for some crates in the kernel repo and for some not
there code around in the kernel repo which doesn't compile

This is something you get for free when you just use cargo but now require additional work (adding some command somewhere in a Makefile). I tried to fix that by making the kernel repo a cargo workspace but gave up. This is exactly the maintance hell you get into when you use Makefiles.

I would agree to having a Makefile in the repo because people coming from the embedded world feel that this takes away some initial hurdles. But the build system should still be make-free so it can be easily maintained by people having only background in Rust (at least this is a Rust library).

torfmaster · 2020-03-11T07:47:35Z

Right now you can build the OpenTitan platform, but accidentally enable atomics via the target flag, which results in a binary that will crash. There is no way that is clear to users and is prone to errors.

I believe this could be fixed by a validation in ´build.rs` We just were not aware of this issue.

.travis.yml

Makefile

.cargo/config

Signed-off-by: Alistair Francis <[email protected]>

alistair23 · 2020-03-11T17:04:00Z

I have addressed all comments

Woyten · 2020-03-12T10:48:13Z

Makefile

+ @echo " - hail"
+ @echo " - nrf52840"
+ @echo " - opentitan"
+ @echo " - hifive1"
+ @echo " - nrf52"


What I really like about this PR is that the platform names have been cleaned up. Before, it was a mixture of CPU architectures and board layouts.

Exactly! :)

torfmaster · 2020-03-13T09:13:01Z

The cleanup is really nice and I have the feeling that many find the Makefile helpful.

bors d+

bors · 2020-03-13T09:13:02Z

✌️ alistair23 can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

alistair23 · 2020-03-13T17:23:05Z

bors r+

bors · 2020-03-13T17:33:09Z

Build succeeded

continuous-integration/travis-ci/push

alistair23 added 2 commits March 9, 2020 17:03

.gitignore: Improve the gitignore file

34103a5

Signed-off-by: Alistair Francis <[email protected]>

boards: Move board layout files into subdir

b3e3a9a

Signed-off-by: Alistair Francis <[email protected]>

labbott reviewed Mar 10, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

torfmaster mentioned this pull request Mar 10, 2020

[RFC] Build systems improvements #162

Open

5 tasks

Woyten reviewed Mar 10, 2020

View reviewed changes

alistair23 force-pushed the alistair/cleanup branch from 4eb378e to 1cc6a8c Compare March 10, 2020 21:16

alistair23 force-pushed the alistair/cleanup branch from 1cc6a8c to a3bd8c4 Compare March 10, 2020 21:56

hifive1: Rename the layout file

b3f213d

Signed-off-by: Alistair Francis <[email protected]>

alistair23 force-pushed the alistair/cleanup branch from a3bd8c4 to 3d01de5 Compare March 10, 2020 22:42

Woyten reviewed Mar 10, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

alistair23 force-pushed the alistair/cleanup branch from 3d01de5 to daa0fea Compare March 10, 2020 22:58

Woyten reviewed Mar 10, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

alistair23 force-pushed the alistair/cleanup branch from daa0fea to 28c3fef Compare March 11, 2020 01:11

torfmaster reviewed Mar 11, 2020

View reviewed changes

README.md Show resolved Hide resolved

torfmaster reviewed Mar 11, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

Woyten reviewed Mar 11, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

labbott reviewed Mar 11, 2020

View reviewed changes

.cargo/config Outdated Show resolved Hide resolved

alistair23 added 10 commits March 11, 2020 09:50

Makefile: Initial commit of a Makefile

9f0b5ee

Signed-off-by: Alistair Francis <[email protected]>

.travis.yml: Trigger Travis on all branches

3badb19

Signed-off-by: Alistair Francis <[email protected]>

Makefile: Split out a setup target

c6fef95

Signed-off-by: Alistair Francis <[email protected]>

rustfmt.toml: Remove obsolete lines

1ff91e7

Signed-off-by: Alistair Francis <[email protected]>

Makefile: Support features

55e3d6b

Signed-off-by: Alistair Francis <[email protected]>

Makefile: Add flash.sh support

cb3d5b8

Signed-off-by: Alistair Francis <[email protected]>

Makefile: Allow the debug build

bb437f6

Signed-off-by: Alistair Francis <[email protected]>

README: Update the README to explain how to get started

0c1cb58

Signed-off-by: Alistair Francis <[email protected]>

docs: Remove outdated README

6695e0d

Signed-off-by: Alistair Francis <[email protected]>

CONTRIBUTING: Update to use the new Make steps

cc49bf0

Signed-off-by: Alistair Francis <[email protected]>

alistair23 force-pushed the alistair/cleanup branch 2 times, most recently from e40e63e to cc49bf0 Compare March 11, 2020 17:03

Woyten approved these changes Mar 11, 2020

View reviewed changes

Woyten reviewed Mar 12, 2020

View reviewed changes

bors bot merged commit 4d27d24 into tock:master Mar 13, 2020

alistair23 deleted the alistair/cleanup branch March 20, 2020 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup and add a Makefile #161

Cleanup and add a Makefile #161

alistair23 commented Mar 10, 2020

labbott left a comment

torfmaster commented Mar 10, 2020

alistair23 commented Mar 10, 2020

ppannuto commented Mar 10, 2020 via email

alistair23 commented Mar 10, 2020

Woyten left a comment •

edited

Loading

Woyten Mar 10, 2020

alistair23 Mar 10, 2020

bradjc commented Mar 10, 2020

alistair23 commented Mar 10, 2020

Woyten commented Mar 10, 2020

torfmaster commented Mar 10, 2020

Woyten Mar 10, 2020

alistair23 Mar 10, 2020

Woyten Mar 10, 2020

alistair23 Mar 11, 2020

Woyten commented Mar 10, 2020

alistair23 commented Mar 11, 2020

torfmaster commented Mar 11, 2020

torfmaster commented Mar 11, 2020

alistair23 commented Mar 11, 2020

Woyten Mar 12, 2020

alistair23 Mar 12, 2020

torfmaster commented Mar 13, 2020

bors bot commented Mar 13, 2020

alistair23 commented Mar 13, 2020

bors bot commented Mar 13, 2020

Cleanup and add a Makefile #161

Cleanup and add a Makefile #161

Conversation

alistair23 commented Mar 10, 2020

labbott left a comment

Choose a reason for hiding this comment

torfmaster commented Mar 10, 2020

alistair23 commented Mar 10, 2020

ppannuto commented Mar 10, 2020 via email

alistair23 commented Mar 10, 2020

Woyten left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradjc commented Mar 10, 2020

alistair23 commented Mar 10, 2020

Woyten commented Mar 10, 2020

torfmaster commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Woyten commented Mar 10, 2020

alistair23 commented Mar 11, 2020

torfmaster commented Mar 11, 2020

torfmaster commented Mar 11, 2020

alistair23 commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torfmaster commented Mar 13, 2020

bors bot commented Mar 13, 2020

alistair23 commented Mar 13, 2020

bors bot commented Mar 13, 2020

Build succeeded

Woyten left a comment •

edited

Loading