radareorg / r2pm

Radare2 cross platform package manager
GNU Lesser General Public License v3.0
34 stars 12 forks source link

fix info struct for new package format #69

Closed imyxh closed 3 years ago

imyxh commented 4 years ago

Detailed description

Most of this commit is just dealing with a rewritten Info struct to handle the new package format, which caused these other changes in the code:

@qbarrand, what do you think? This is my first time writing Go and contributing to this project so quite frankly I'm sure I screwed something up. I think my approach could be modified, perhaps, to fit in better with package.go? Not sure where you plan to go with that.

...

Test plan

A test package for r2dec-js installs with this YAML:

---
name: r2dec-test
version: 0.0.0
description: "Converts asm to pseudo-C code. Test package for new format."

install:

  linux:
    source:
      type: git
      repo: https://github.com/radareorg/r2dec-js.git
      ref: master
    commands:
        - make -C p

# TODO: uninstall, windows, etc.
# TODO: {{ .Variable }} syntax parsing

# vim: ft=yaml sw=2:

...

Closing issues

None yet.

...

qbarrand commented 4 years ago

Great job, Ian!

I'd be more comfortable if we added support for all OSes before merging, though.

Wondering also why the GitHub action was not triggered....

imyxh commented 4 years ago

I'd be more comfortable if we added support for all OSes before merging, though.

Sure. :)

Wondering also why the GitHub action was not triggered....

Hmm, perhaps the actions aren't set up for the new-package-format branch?

XVilka commented 4 years ago

By the way, I suggest to create some kind of test package installing nothing, just checking if it parsed successfully and all actions can be done.

Also do not forget tags support in the package format:

imyxh commented 4 years ago

Test package should be up at imyxh/r2pm-test. Eventually, perhaps we could make a template package that does nothing under radareorg. Still working on the code :)

imyxh commented 4 years ago

@qbarrand full support according to spec is now built in to the info struct, it now just has to be implemented elsewhere (i.e. write the implementation for zip install and handle out files).

Sorry it took so long for something so simple. If you think it's good to merge into the new-package-format branch, then we can close this PR, otherwise I can also keep working on implementation.

Edit: note that I added tags support as well.

Edit 2: shit, it's been so long I forgot I need to stop hardcoding Linux before we merge. One moment.

Edit 3: okay, I've implemented some basic platform detection. Check the commit message (8c71ae58b70fedd9395fb0df9c67e85e74dbb800) for an overview.

XVilka commented 4 years ago

LGTM. @qbarrand ok to merge?

XVilka commented 4 years ago

@imyxh have you had any updates on this one?

imyxh commented 4 years ago

@XVilka ooh, thanks for the reminder! I think where this left off was finished support for Linux, Windows, MacOS but no clean way to support anything else as we can't (afaik) create struct fields on the fly: https://github.com/radareorg/r2pm/pull/69#discussion_r409890629

Edit: oops, hit comment too soon

imyxh commented 4 years ago

If anyone has a good idea on how best to selectively unmarshal the correct YAML tags based on current operating system I'd love a pointer in the right direction.

XVilka commented 4 years ago

@qbarrand since your new format changes are already in the master - should be this closed or reworked? Please take a look again.

imyxh commented 3 years ago

After skimming the new changes I think I am pretty confident that this merge request is incompatible with the recent work. I'm not sure atm how the selective unmarshaling by GOOS is done, but I trust qbarrand's code is probably cleaner than what I would've done. Closing!

qbarrand commented 3 years ago

Cheers @imyxh. #74 gets a user-friendly OS family, and uses that value to get the per-family installation instructions from a map. Although I think that solution is more flexible, if was heavily inspired by your work in this MR. Thank you!