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

common.mk: Fix BASEDIR to manage github-action workspace directory #6066

Merged
merged 1 commit into from
Apr 6, 2024

Conversation

th0ma7
Copy link
Contributor

@th0ma7 th0ma7 commented Apr 6, 2024

Description

common.mk: Fix BASEDIR to manage github-action workspace directory.

When using github-action the spksrc repository sits under /github/workspace. Therefore the BASEDIR value set in spksrc.common.mk is not taking this into account, making publishing to file as make setup-synocommunity had failed previously with the following:

2024-04-06T10:42:28.7394231Z ##[group] ---- initialize build
2024-04-06T10:42:29.5904113Z mk/spksrc.common.mk:59: ../../mk/spksrc.archs.mk: No such file or directory
2024-04-06T10:42:29.5905666Z make: *** No rule to make target '../../mk/spksrc.archs.mk'.  Stop.
2024-04-06T10:42:29.7350751Z sed: can't read local.mk: No such file or directory
2024-04-06T10:42:29.7510857Z ##[endgroup]

Above that error can be seen the actual path being used by github-action:

"/github/file_commands" -v "/home/runner/work/spksrc/spksrc":"/github/workspace" ghcr.io/synocommunity/spksrc:latest

Fixes: #6061
Follow-up to: #6064 (thnx @hgy59) and broken by #6002

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix
  • New Package
  • Package update
  • Includes small framework changes
  • This change requires a documentation update (e.g. Wiki)

@th0ma7
Copy link
Contributor Author

th0ma7 commented Apr 6, 2024

@hgy59 @mreid-tt and @Safihre : I believe this should solve the remaining issue. Tested locally by symlinking workspace -> spksrc and local.mk files gets generated as expected.

@th0ma7 th0ma7 requested review from hgy59 and mreid-tt April 6, 2024 13:40
@th0ma7 th0ma7 self-assigned this Apr 6, 2024
@hgy59
Copy link
Contributor

hgy59 commented Apr 6, 2024

I hate such hard coded values, but can't imagine another solution...

But for the test targets, we should not use mk/spksrc.test-rules.mk

@th0ma7 th0ma7 mentioned this pull request Apr 6, 2024
10 tasks
@th0ma7
Copy link
Contributor Author

th0ma7 commented Apr 6, 2024

I hate such hard coded values, but can't imagine another solution...

Likewise.

But for the test targets, we should not use mk/spksrc.test-rules.mk

Why so? Any other option that comes to mind? Besides merging it into the spksrc/Makefile which I would hate to see there.

@mreid-tt
Copy link
Contributor

mreid-tt commented Apr 6, 2024

This is a bit over my head but seems sensible.

@hgy59
Copy link
Contributor

hgy59 commented Apr 6, 2024

Why so? Any other option that comes to mind? Besides merging it into the spksrc/Makefile which I would hate to see there.

Because spksrc-test-rules.mk is designed with another scope. It must be included by spksrc/Makefile only and not by make files for cross, spk and diyspk packages.

It's the ugly include mk/spksrc.common.mk that I'm complaining about in spksrc-test-rules.mk.
Even when you can remove this include, I would recommend to move spksrc-test-rules.mk to a different folder like spksrc/test.

But why not creating a new make file like /spksrc/Makefile.test ?

@mreid-tt mreid-tt removed their request for review April 6, 2024 14:25
@th0ma7
Copy link
Contributor Author

th0ma7 commented Apr 6, 2024

Ok, coming up with something as #6065 (comment) will take me a little longer. I'll have a look at it over the coming days and create a PR (or reuse yours @hgy59) so it uses the default publish errror callout.

In the meantime I belive this simple fix fixes the immediate issue. I'll merge this as-is and let's confirm it works ok.

I suggest we look into adding the other enhancements elsewhere, possibly into your PR #6065 directly as this would keep the discussion thread:

  1. Potentially using a Makefile.test or similar
  2. make publish-arch* standard error

Are we ok with this proposal?

@hgy59
Copy link
Contributor

hgy59 commented Apr 6, 2024

Are we ok with this proposal?

I will merge #6065 with changes of spksrc.publish.mk only.

The other tasks shold be done in dedicated PRs.

Copy link
Contributor

@hgy59 hgy59 left a comment

Choose a reason for hiding this comment

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

LGTM

@th0ma7 th0ma7 merged commit 5d24d94 into SynoCommunity:master Apr 6, 2024
1 check passed
@th0ma7 th0ma7 deleted the fix-publishing branch April 6, 2024 15:10
@hgy59
Copy link
Contributor

hgy59 commented Apr 6, 2024

@th0ma7 sorry, but after my approval, I found the same definition in spksrc.arch.mk:

# Set basedir in case called from spkrc/ or from normal sub-dir
ifeq ($(BASEDIR),)
ifneq ($(shell basename $(CURDIR)),spksrc)
BASEDIR = ../../
endif
endif

sould it be fixed too?

@th0ma7
Copy link
Contributor Author

th0ma7 commented Apr 6, 2024

yes, let me do, yet another pr... sigh... and good catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants