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.

Sign up for GitHub

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

Jump to bottom

rearrange tree #489

Closed
wants to merge 1 commit into from
Closed

rearrange tree #489

wants to merge 1 commit into from

Conversation

williamh
Copy link
Contributor

This PR does several things to streamline the source tree.

  • drop old build system
  • move shared include files to common directory
  • move shared source files to common directory
  • move executable-specific code to separate directories
  • move scripts to separate directories under src

All of this should not result in any user facing changes. Please review.

@vapier @thesamesam

@williamh williamh force-pushed the rearrange-tree branch 4 times, most recently from 1ecba54 to 33f6a72 Compare December 31, 2021 01:26
vapier
vapier reviewed Dec 31, 2021
View reviewed changes
.gitignore Outdated Show resolved Hide resolved
meson.build Outdated
@@ -181,7 +182,7 @@ cc_warning_flags = cc.get_supported_arguments(cc_warning_flags_test)
cc_flags = [cc_debug_flags, cc_os_flags, cc_warning_flags]
add_project_arguments(cc_flags, language : 'c')

incdir = include_directories('src/includes')
incdir = include_directories('src/common')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think collapsing includes/ into common/ is a good pattern. having a dir dedicated to headers for APIs that are meant to be reused across the codebase helps enforce good boundaries. when the dir mixes such "exported" APIs with internal headers, it's hard for other dirs to know what's "internal", and what should be kept that way.

basically, the existence of a header file in a dir should not be a signal by itself that it's OK for reuse outside of the dir.

src/common/meson.build Outdated
@@ -1,6 +1,35 @@
rc_misc_c = files([
'rc-misc.c',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since these files are in common/ now, do they still really need the rc- prefix on the filename ?

meson.build Outdated Show resolved Hide resolved
src/checkpath/meson.build
@@ -0,0 +1,9 @@
executable('checkpath',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm having a hard time seeing why per-program subdirs is an improvement

meson.build
@@ -201,7 +201,6 @@ subdir('init.d')
subdir('local.d')
subdir('man')
subdir('pkgconfig')
subdir('scripts')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as the compiled code ... having a hard time seeing this as improvement and not just a lot of busywork

@williamh williamh requested a review from vapier December 31, 2021 17:53
@williamh
Copy link
Contributor Author

williamh commented Jan 3, 2022

@vapier I'm having a11y issues with the individual review comments, so I'll respond here.

  • The first comment in your review is fixed; I addressed this on the master branch.
  • I don't understand the second one. All code in "common" is shared across the code base.
  • No, the files in common do not need the rc_prefix. I'll take care of that in another pr.
  • The fourth one is addressed the same way the first one was, I moved that to master.
  • I agree that the fifth and sixth ones are somewhat subjective, but I feel that it makes the source cleaner to have one directory of common files used across the code base and separate directories for the executable-specific code.

@williamh
Copy link
Contributor Author

williamh commented Jan 3, 2022

@vapier @thesamesam I would like to merge this farely soon, so can you take a look and comment?

@vapier
Copy link
Member

vapier commented Mar 6, 2022

prob want to go through all open comment threads and close them out

i don't see anything wrong with the changes other than the existing high level feedback i left already

@williamh
migrate fully to meson build system
23854a7 
- drop old build system
- move shared include and source files to common directory
- drop "rc-" prefix from shared include and source files
- move executable-specific code to individual directories under src
- adjust top-level .gitignore file for new build system
@williamh williamh closed this in 391d12d Apr 6, 2022
@williamh williamh deleted the rearrange-tree branch April 6, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vapier vapier Awaiting requested review from vapier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@williamh @vapier