Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Fixes for various make:all travis failures #4864

Closed
wants to merge 3 commits into from

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Jan 18, 2019

Description

#4862 has highlighted some existing failures, to my current understanding of the issues, these are as follows:

hadron

  • rev0 seems to have no rules.mk content and keymap folder
  • all revisions have same layout format, however there seems to be some differences
  • potential solution?
    • refactor to usual "revision" layout
      • This means moving and at least modifying the default keymap
    • or add rules, keymap + default folder

handwired/dactyl_manuform

  • set up to reuse base config for multiple different layouts
  • potential solution?
    • migrate base config to each board
    • or fix makefile all rule
      • somehow detect that it is just a parent folder (see below)

lfkeyboards

  • Set up to add default src files to multiple different boards
  • Most boards already add the additional src files
    • remove base config and add config to single board that requires the extra files

bigseries

  • Set up to reuse base config for multiple different boards
  • Potential solution?
    • migrate base config to each board
    • or fix makefile all rule
      • somehow detect that it is just a parent folder (see below)

Potential ideas for 'somehow detect that it is just a parent folder'

  • some environment differences between docker and travis
    • will investigate further in this area for isolated fix, toolchain differences, etc
  • only required for make all
  • ignore make rule when
    • parent contains folder with rules.mk
    • parent contains no default folder

Types of changes

  • Core
  • Bugfix
  • New Feature
  • Enhancement/Optimization
  • Keyboard (addition or update)
  • Keymap/Layout/Userspace (addition or update)
  • Documentation

Issues Fixed or Closed by this PR

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document. (https://docs.qmk.fm/#/contributing)
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna
Copy link
Member

drashna commented Jan 18, 2019

Honestly, I'm not really a fan of doing this.

I don't feel that we should be changing things around to get compiling to work properly on Docker, if it works just fine "normally". I feel that trying to figure out why it's erroring out and fixing that is more important, than changing things to work with it.

@zvecr
Copy link
Member Author

zvecr commented Jan 18, 2019

@drashna in theory this isnt a just a docker issue, especially when some of the required changes are broken code (hadron/rev0 currently doesnt compile). It should fail currently on master, but only when the right conditions are met. Currently the build scripts detect changes made to the repo, and do not run make all when its just stuff like a keymap change Given some digging through the current PRs, it looks like the issues are isolated to the docker builds, however the lfkeyboards and hadron issues are masked by the differences.

@zvecr
Copy link
Member Author

zvecr commented Jan 18, 2019

To add, i would also be hesitant with the keymap changes and favour the simpler fixes but wanted to list them for completeness, and so that someone more in the know could help shape the direction of any potential fixes.

Is there a way to trigger a travis make all on the current master to categorically see if we have the same issues? Recent PRs (such as #4842) do not suffer this issue.

If required i can pull out the important parts from this investigative work into a per keyboard PR.

@zvecr
Copy link
Member Author

zvecr commented Jan 20, 2019

Fixes for hadron/rev0 and lfkeyboards have been completed, the other boards mentioned in the opening comments have been left pending a fix within the docker environment.

Would the current changes be accepted? would it be better as a PR per keyboard?

@zvecr
Copy link
Member Author

zvecr commented Jan 21, 2019

@drashna now i have a potential fix for #4862, would you advise splitting the hadron/rev0 and lfkeyboards changes into 2 separate PRs?

@drashna
Copy link
Member

drashna commented Jan 22, 2019

I think this should be okay.

@zvecr
Copy link
Member Author

zvecr commented Jan 23, 2019

@ishtob exactly the kind of thing i like to hear, will make the change right now.

@zvecr
Copy link
Member Author

zvecr commented Jan 23, 2019

Closing this PR and opening 2 separate ones for hadron and lfkeyboards fixes, i think its cleaner now that the discussion around this PR and #4862 have reached resolution.

@zvecr zvecr closed this Jan 23, 2019
@zvecr zvecr deleted the feature/travis_failures branch April 28, 2020 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants