search

manna-harbour / miryoku

Miryoku is an ergonomic, minimal, orthogonal, and universal keyboard layout.
2.47k stars 118 forks source link

[Bug] Generating with org-babel-tangle litters empty main functions #41

Closed tarkpate closed 2 years ago

tarkpate commented 3 years ago

Steps to reproduce: -Open miryoku.org in emacs -Run org-babel-tangle with C-c C-v t

Operating Systems: -Arch Linux -Mac 10.14

When generating with babel tangle, many files have an empty main function appended. For example, the file /layouts/community/split_3x6_3/manna-harbour_miryoku/config.h:

int main() {
// generated from users/manna-harbour_miryoku/miryoku.org  -*- buffer-read-only: t -*-

#pragma once

#define LAYOUT_miryoku(\
       K00,   K01,   K02,   K03,   K04,          K05,   K06,   K07,   K08,   K09,\
       K10,   K11,   K12,   K13,   K14,          K15,   K16,   K17,   K18,   K19,\
       K20,   K21,   K22,   K23,   K24,          K25,   K26,   K27,   K28,   K29,\
       N30,   N31,   K32,   K33,   K34,          K35,   K36,   K37,   N38,   N39\
)\
LAYOUT_split_3x6_3(\
KC_NO, K00,   K01,   K02,   K03,   K04,          K05,   K06,   K07,   K08,   K09,   KC_NO,\
KC_NO, K10,   K11,   K12,   K13,   K14,          K15,   K16,   K17,   K18,   K19,   KC_NO,\
KC_NO, K20,   K21,   K22,   K23,   K24,          K25,   K26,   K27,   K28,   K29,   KC_NO,\
                     K32,   K33,   K34,          K35,   K36,   K37\
)
return 0;
}

This is a list of the files that have this appended to it: keyboards/atreus/keymaps/manna-harbour_miryoku/config.h keyboards/atreus/keymaps/manna-harbour_miryoku/keymap.c keyboards/for_science/keymaps/manna-harbour_miryoku/config.h keyboards/for_science/keymaps/manna-harbour_miryoku/keymap.c keyboards/gergo/keymaps/manna-harbour_miryoku/config.h keyboards/gergo/keymaps/manna-harbour_miryoku/keymap.c keyboards/keebio/iris/keymaps/manna-harbour_miryoku/config.h keyboards/keebio/iris/keymaps/manna-harbour_miryoku/keymap.c keyboards/kyria/keymaps/manna-harbour_miryoku/config.h keyboards/kyria/keymaps/manna-harbour_miryoku/keymap.c keyboards/lily58/keymaps/manna-harbour_miryoku/config.h keyboards/lily58/keymaps/manna-harbour_miryoku/keymap.c keyboards/moonlander/keymaps/manna-harbour_miryoku/config.h keyboards/moonlander/keymaps/manna-harbour_miryoku/keymap.c keyboards/redox_w/keymaps/manna-harbour_miryoku/config.h keyboards/redox_w/keymaps/manna-harbour_miryoku/keymap.c layouts/community/60_ansi/manna-harbour_miryoku/config.h layouts/community/60_ansi/manna-harbour_miryoku/keymap.c layouts/community/ergodox/manna-harbour_miryoku/config.h layouts/community/ergodox/manna-harbour_miryoku/keymap.c layouts/community/ortho_4x12/manna-harbour_miryoku/config.h layouts/community/ortho_4x12/manna-harbour_miryoku/keymap.c layouts/community/ortho_5x15/manna-harbour_miryoku/config.h layouts/community/ortho_5x15/manna-harbour_miryoku/keymap.c layouts/community/planck_mit/manna-harbour_miryoku/config.h layouts/community/planck_mit/manna-harbour_miryoku/keymap.c layouts/community/split_3x5_3/manna-harbour_miryoku/config.h layouts/community/split_3x5_3/manna-harbour_miryoku/keymap.c layouts/community/split_3x6_3/manna-harbour_miryoku/config.h layouts/community/split_3x6_3/manna-harbour_miryoku/keymap.c users/manna-harbour_miryoku/config.h users/manna-harbour_miryoku/manna-harbour_miryoku.c users/manna-harbour_miryoku/manna-harbour_miryoku.h

manna-harbour commented 3 years ago

Do you have any org-babel customisations, particularly f'or C? What version of org are you using?

manna-harbour commented 3 years ago

Try adding :main no to the header arguments.

tarkpate commented 3 years ago

Thanks.That fixed it so I'll make a PR.

manna-harbour commented 3 years ago

Thanks. What org version are you using? There have been a few random changes with new org versions recently.

obar commented 3 years ago

I see the same extra main wrappers using Org 9.4, and @RubioJr9's branch solves it. However I can't definitively say this should be merged as I'm also getting the extra commas mentioned in manna-harbour/miryoku#30—will elaborate on that issue.

obar commented 3 years ago

I played around with this and I'd say merge :+1:

manna-harbour commented 3 years ago

@RubioJr9 @obar Thanks for creating and investigating this issue. I've updated org from 9.1.9 to 9.4.5 and I don't see this issue in either version. Can you please test with org 9.4.5 and also tell me what emacs and python versions you are using?

obar commented 3 years ago

Fairly current versions of everything here, Emacs 27 / Org 9.4 / Python 3.8.5

I needed the :main no tweak from manna-harbour/qmk_firmware#12 to make it work. It doesn't appear that any recent changes to Org's c handling would have affected this behavior. @manna-harbour is it possible you have changed the variable org-babel-header-args:C somewhere in your config?

tarkpate commented 3 years ago

I'm still having this issue. Emacs 27.2 / Org 9.5 / Python 3.9.3

manna-harbour commented 3 years ago

Fairly current versions of everything here, Emacs 27 / Org 9.4 / Python 3.8.5

I needed the :main no tweak from manna-harbour/qmk_firmware#12 to make it work. It doesn't appear that any recent changes to Org's c handling would have affected this behavior. @manna-harbour is it possible you have changed the variable org-babel-header-args:C somewhere in your config?

I haven't changed org-babel-header-args:C.

I'm still having this issue. Emacs 27.2 / Org 9.5 / Python 3.9.3

With manna-harbour/qmk_firmware#10 merged and emacs 26 I've tried the following with no main wrapping issue present:

I'll try with a more recent emacs.

obar commented 3 years ago

Interesting results. Could this have been changed by some other package / what do you get from running C-h v org-babel-header-args:C?

manna-harbour commented 3 years ago

Interesting results. Could this have been changed by some other package / what do you get from running C-h v org-babel-header-args:C?

org-babel-header-args:C is a variable defined in ‘ob-C.el’.
Its value is
((includes . :any)
 (defines . :any)
 (main . :any)
 (flags . :any)
 (cmdline . :any)
 (libs . :any))

With emacs 27.1, python 3.9.4, org 9.4.5, still no main wrapping issue. Also tried with a clean emacs config. It must be a library issue... I'll try with an updated system.

manna-harbour commented 3 years ago

Tried in a container, still no main wrapping issue:

Ubuntu 20.04.2 emacs 26.3, org 9.1.9, python 3.9.0+ emacs 26.3, org 9.4.5, python 3.9.0+ emacs 27.1, org 9.4.5, python 3.9.0+

manna-harbour commented 3 years ago

Here it is in ubuntu-latest with default emacs / org / python3, with no main wrapping issue:

Workflow: https://github.com/manna-harbour/qmk_firmware/actions/workflows/tangle.yml

Last run: https://github.com/manna-harbour/qmk_firmware/runs/2574409943

manna-harbour commented 3 years ago

@RubioJr9 @obar Would you mind testing with a clean emacs config?

tarkpate commented 3 years ago

I'll test this now. Regardless whether the issue persists, this PR should be merged in because disabling the main wrapping explicitly is more robust. The defaults may not stay the same between current and future versions.

obar commented 3 years ago

Just tested in emacs -Q, ie no user config, told babel to use python3 (3.8.5), and still got the same main wrapping. I would agree that it makes sense to put this in explicitly in any case.

Looking at ob-C.el I really don't see how the main wrapping wouldn't be happening. At any rate it doesn't give you a negative effect to have that patch, correct?

tarkpate commented 3 years ago

I'm not getting this behavior anymore with Org 9.5 and python 3.9.5

obar commented 3 years ago

I hadn't fully thought this through with my emacs -Q test: I didn't load the latest org, I used the one that emacs had built-in (9.3). I just updated to the latest org on ELPA (9.4.6) and with it I'm still getting the wrapper.

I don't think the python version should matter for this issue. Should it?

Could the wrapper's absence be an indication of a bug elsewhere? I haven't seen any indication that the default behavior was changed.

manna-harbour commented 3 years ago

Re https://github.com/manna-harbour/qmk_firmware/pull/12#issue-539831896, I've just heard that a single #+PROPERTY: header-args :main no fixes this issue in org 9.4.6. @obar Would you mind trying that as an alternative to :main no in every source block? As I can't reproduce the bug I can't verify if that works. If it does I'll add it, but if not I'll just add the PR as is.

manna-harbour commented 3 years ago

I just updated to the latest org on ELPA (9.4.6) and with it I'm still getting the wrapper.

I don't think the python version should matter for this issue. Should it?

Could the wrapper's absence be an indication of a bug elsewhere? I haven't seen any indication that the default behavior was changed.

Thanks. It must be something to do with the platform or other libraries then, Python version only doesn't seem to matter as I've tested multiple versions. Behaviour of babel has changed before without documentation so it's hard to tell if it's intentional or not...

tarkpate commented 3 years ago

Re https://github.com/manna-harbour/qmk_firmware/pull/12#issue-539831896, I've just heard that a single #+PROPERTY: header-args :main no fixes this issue in org 9.4.6. @obar Would you mind trying that as an alternative to :main no in every source block? As I can't reproduce the bug I can't verify if that works. If it does I'll add it, but if not I'll just add the PR as is.

I tried this with an older org version before I made the PR, but it didn't work. It'd certainly be worth trying on the newer version though

manna-harbour commented 3 years ago

I tried this with an older org version before I made the PR, but it didn't work. It'd certainly be worth trying on the newer version though

Yes that's what I was referring to by the linked comment, and thanks for trying it already!

I just managed to trigger main wrapping once by some combination of enabling C code block execution, adding and executing a test block, and tangling, but I haven't been able to reproduce it. Also, using the PROPERTY to enable main wrapping (which should be automatic) didn't work for me, so I'm going to assume it won't necessarily work to disable it.

I'm just going to merge the existing fix and move on. Thanks everyone!