Skip to content
This repository has been archived by the owner on Jun 16, 2022. It is now read-only.

Add rice-append task #108

Merged
merged 1 commit into from
Aug 21, 2016
Merged

Add rice-append task #108

merged 1 commit into from
Aug 21, 2016

Conversation

pdf
Copy link
Collaborator

@pdf pdf commented Aug 13, 2016

Adds a task to call rice append on the build result, allowing
embedding of resources into the binary.

@laher
Copy link
Owner

laher commented Aug 20, 2016

Hey there. Thanks for your PR, it looks like a great feature.

Unfortunately, it can't be merged in yet because it has been brought into the 'default' tasks and makes no accommodation for the possibility that someone hasn't got rice on their system, or doesn't want to run it. It's just because you added it into TASKS_COMPILE, and those tasks are all run as part of TASKS_DEFAULT. There are people using this tool who would run go get -u and find that their build suddenly breaks.

See https://github.com/laher/goxc/blob/master/tasks/tasks.go#L80

Please remove it from TASKS_COMPILE and into TASKS_OTHER. That would be an easy way to have it merged for now. People who wanted to use rice would then add the extra task into their config (or commandline args).

_NOTE: An alternative would be to assume that someone who hasn't set "import-paths" probably doesn't want to run the tool anyway. So, that'd be an acceptable. I'm not sure it would belong with TASKS_COMPILE though. _

Vague note: it'd also be nice to check if the rice command exists. Not necessarily straightforward to discover executables on the PATH but something to keep in mind. Maybe just running go get would do it (it should be idempotent to run it each time this task is run)

Cheers

Adds a task to call `rice append` on the build result, allowing
embedding of resources into the binary.
@pdf
Copy link
Collaborator Author

pdf commented Aug 20, 2016

Good notes, thank you - should be addressed now.

@laher
Copy link
Owner

laher commented Aug 21, 2016

LGTM. Please merge, thanks

@pdf pdf merged commit f8fbb3b into laher:master Aug 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants