First implementation of piconim supporting autolinking of libraries

[beef331]

@casey-SK @auxym if you can please take a look and try this out. This is an alternative to the way Casey proposed.

[beef331]

Thinking about it further there is less hacky way around but would require the following though.

include picostdlib

[auxym]

I haven't tried it yet (will later), but looked at the diff. Two comments:

1. If you're modifying CMakeLists.txt, you have to re-run CMake afterwards. Or at least that is my understanding. CMake will re-generate the Makefile with the correct includes etc. I don't see anywhere piconim build would be running Cmake in the current code.

2. I (personally) would strongly prefer that piconim would not not touch the CMakeLists file. As a dev that's a file I might want to modify manually and keep around, eg, adding "external" libraries to it. The current implementation would nuke that (I think). My suggestion would be that piconim writes a second file (can be nuked, gitignored, etc) that is included in CMakeLists.txt. Always included, no modifications needed to CMakeLists.txt file.

[beef331]

1) I do not know if that's the case I've been using piconim and manually adding the includes, I think the make file knows when the cmake is modified and handles it. I could be wrong I've never used cmake/make before really.

2) I think that's a fair point but as I do not know cmake I do not know how to do as such.

[auxym]

It seems you're right (I really don't know much about cmake and make myself). CMake generates Makefile, which runs cmake, which generates Makefile2, which actually does the build (a bit convoluted, but ok pico-sdk, I guess).

For the include, it shouldn't be too hard (I think, literally I haven't used cmake before). Look at the current CMakeLists file:

include(pico_sdk_import.cmake)

I'd suggest that your new addLinkLibs steps just write the target_link_libraries(...) entries in a separate file (eg. pico_libraries.cmake) and this file be included in the CMakeLists.txt file (just included in the original template, not need to modify it afterwards):

include(pico_libraries.cmake)

[casey-SK]

Hello, sorry it took a while. I looked at what you did and implemented @auxym 's recommendation. The following branch includes all changes from #37 and #36

https://github.com/casey-SK/picostdlib/tree/feature_improve_piconim_debug

Notes:

I have changed the subcommand names:

• create essentially just copies the template folder
• init initializes the local deps, namely pico-sdk and nimbase.h
• build does what it did before, no major changes

addLinkLibs now places in the libs into a new pico_libraries.cmake file, which is include()ed in the CMakeLists.txt file. instead of dealing with parsing, it just adds a new line. According to the cmake documentation, this is fine.

I also added in a lot more feedback to the user, via printMessage, as well as more documentation

let me know if I should create a PR of this branch.

[beef331]

@casey-SK good work, but I'm leaning to just searching for picostdlib@picostdlib@stdio.c for example in the csource directory now, since this is a very big hack and ugly. This does mean every module added needs to be supported in piconim, but it's a cleaner solution than this PR proposed, this was just an implementation to see what was thought of.

[casey-SK]

when you say picostdlib@picostdlib@stdio.c , I am not quite sure what you mean by that. I don't see any files that match that, or come near to matching that syntax. I suppose I will get a better understanding once you implement it

[beef331]

The Nim generated code that comes from this package should end with that(atleast on linux).

[casey-SK]

The generated files I am familiar with start with @m and end with .nim.c:

https://github.com/casey-SK/picostdlib/blob/fae32f5557762766ae579be51063f0ce46830002/src/piconim.nim#L225

[beef331]

Yea actually attempting to implement it this will not work, so I guess scanning for #include is the way to go.

[beef331]

Here is a semi-clean/extendable solution to the include, now to figure out how to use it in cmake.

[casey-SK]

Here is a semi-clean/extendable solution to the include, now to figure out how to use it in cmake.

Here is how I solved your previous implementation using cmake:

https://github.com/casey-SK/picostdlib/blob/fae32f5557762766ae579be51063f0ce46830002/src/piconim.nim#L34

In your new implementation you are creating a file called imports.txt, instead, just create a file called pico_imports.cmake (or something) and for each line just do: "target_link_libraries(${CMAKE_PROJECT_NAME} " & $lib & ")". Then in the CMakeLists.txt file, instead of target_link_libraries( ... ) , replace it with include(pico_imports.cmake).

[casey-SK]

Here is a refactored implementation of (https://github.com/beef331/picostdlib/pull/37/commits/6f62254c99445eced4920b536f7c6f283e8d4fd4):

https://github.com/casey-SK/picostdlib/commit/7e4028a69ac0c4b5a0a6f4b0991fce100c4fc976

[beef331]

Can you a PR with that version of autolink, can merge it then merge you other things in?

[casey-SK]

Okay just confirm. Here is how my dev branch got to this point:

1. I checked out the beef331:picostdlib:master branch
2. I merged the Jan 9th version of the autolink branch
3. I merged @auxym #36 in and got them to agree with each other
4. I then did a whole lot of refactoring of the code to accomplish the following:
   • change the commands from init, setup, and build -> create, init, build
   • added printMessage() proc, replacing printError
   • added more error handling in areas where it had been missed.
   • I changed your original addLinkLibs() to work with cmake
   • moved the "helper" procs into their respective procs, so now the there are 3 top level procs for create, init, and build + the printMessage() proc. Not sure if it is good practice limiting the scope of these procs in this way. Comments?
5. Then I created a new branch (dev) and copy/pasted your link lib code in place of where addLinkLibs() was and got it working.
6. Added a couple more printMessage() statements

Haha, so with all that said... What is the best way forward? I assume doing a PR of all my changes in one go is not best practice? I guess I need a little more guidance to make sure I take the right steps, the first time.

[auxym]

The implementation of getLinkedLib is somewhat fragile, but given that it is scanning code generated by the nim compiler, which should be somewhat predictable, it's probably OK. Cases I can think of that could break it:

/*
#include header.h
*/

// Note the spaces, I think this is still valid syntax
#  include header.h

IMO: if anything in the nim codegen in the future breaks this, we can fix it later. So it's ok for me as-is.

[casey-SK]

hey @auxym , I am having an issue with cmake stuff. When I create a test project that requires a linked import, addLinkedLibs appears to correctly add the target_link_libraries(), but yet the build fails. However, the build will pass if the following is done:

• rename the pico_libraries.txt (after you have run piconim build at least once)
• change the include() line in CMakeLists.txt to the renamed file

So I know that our program can generate a valid entry, but yet is there some sort of file permission or timing issue going on?

[casey-SK]

Here is my example: https://github.com/casey-SK/tests

notice that I changed pico_libraries.txt to pico_libs.txt and the build now passes. The contents of the two files are the exact same.

[casey-SK]

Okay, I got it working by updating the timestamp on the file before running make: https://github.com/casey-SK/picostdlib/commit/29102123aec8c8de9dc78f75f9bded75b3610071

[casey-SK]

next question: should the file be called pico_libraries.txt or pico_libraries.cmake , or maybe something else? Now is our chance to get it right, lol. Also maybe someone knows why this timestamp thing is so critical?

[auxym]

Hm, weird, I'm not super familier with CMake. That said, piconim is re-writing the file on build, so its timestamp should be updated automatically that way, no?

As for the extension, I'd go with .cmake. Not sure if it's universal convention, but it is the convention used in pico-sdk (see all files in https://github.com/raspberrypi/pico-sdk/tree/master/cmake).

[beef331]

Okay just confirm. Here is how my dev branch got to this point:

1. I checked out the beef331:picostdlib:master branch

2. I merged the Jan 9th version of the autolink branch

3. I merged @auxym [WIP: Add gitignore and setup command #36](https://github.com/beef331/picostdlib/pull/36) in and got them to agree with each other

4. I then did a whole lot of refactoring of the code to accomplish the following:

   * change the commands from `init`, `setup`, and `build`  ->  `create`, `init`, `build`
   * added `printMessage()` proc, replacing `printError`
   * added more error handling in areas where it had been missed.
   * I changed your original `addLinkLibs()` to work with `cmake`
   * moved the "helper" procs into their respective procs, so now the there are 3 top level procs for `create`, `init`, and `build` + the `printMessage()` proc. Not sure if it is good practice limiting the scope of these procs in this way. Comments?

5. Then I created a new branch (dev) and copy/pasted your link lib code in place of where `addLinkLibs()` was and got it working.

6. Added a couple more printMessage() statements

Haha, so with all that said... What is the best way forward? I assume doing a PR of all my changes in one go is not best practice? I guess I need a little more guidance to make sure I take the right steps, the first time.

The best thing to do is separate commits into sequential PRs, this way we can handle each step and easily make each step as clean as possible.

[auxym]

next question: should the file be called pico_libraries.txt or pico_libraries.cmake , or maybe something else? Now is our chance to get it right, lol. Also maybe someone knows why this timestamp thing is so critical?

Oh I think I know what's going on:

As I mentioned earlier, make re-runs cmake. make use file timestamps to determine what it needs to do. make knows of CMakeLists.txt but not its included files (unless they were declared as dependencies explicitly by the makefile).

The way I see it, two options, either of which is fine IMO

• Touch CMakeLists.txt as you're doing to force a rebuild
• Have piconim run cmake .. from the build directory on each build. No need to pass in the sdk option, it gets cached and reused automatically in CMakecache.txt.

[beef331]

@auxym @casey-SK can you try this out now, I obviously tested it but need to know if I made it annoying somewhere. :smile: