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

make avail: check for presence of binaries before creating symbolic links #1994

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

Compyx
Copy link
Contributor

@Compyx Compyx commented Feb 19, 2023

To avoid creating broken symlinks, first check if the binaries exists in bin/ and exit when they don't, with a message to first run make.

links

To avoid creating broken symlinks, first check if the binaries exists in
bin/ and exit when they don't, with a message to first run `make`.
@mrdudz
Copy link
Contributor

mrdudz commented Feb 19, 2023

The idea is good - but i am not sure i like it is using the shell (our Makefiles don't use the shell).

@oliverschmidt any idea how this can be done just using make? I'd have to look it up and play around (but its probably possible... i guess)

@oliverschmidt
Copy link
Contributor

I.e. https://stackoverflow.com/questions/5553352/how-do-i-check-if-file-exists-in-makefile-so-i-can-delete-it

BTW: A consistent name for AVAIL_check_prog would be PROG_CHECK_recipe.

@Compyx
Copy link
Contributor Author

Compyx commented Feb 19, 2023

I tried the ifeq approach first, but the problem with that is that's is evaluated at parse time and we're trying to do the checks inside a rule. But i'll give it another shot.

Use `ifeq` to provide two rules for the `avail` target: one that reports
an error if any of the symlink targets are missing and one that installs
the symlinks for the targets if they're all present.
@rofl0r
Copy link
Contributor

rofl0r commented Feb 19, 2023

you could just foreach( wildcard ../bin ... instead foreach($PROGS)

@Compyx
Copy link
Contributor Author

Compyx commented Feb 20, 2023

I'm not sure what you mean.

Are you talking about the foreach loop that installs the symlinks? I assume the PROGS list is there to make sure it installs only symlinks to those programs, not everything that happens to be in ../bin/. And the current PR doesn't contain a foreach after the last commit.

@rofl0r
Copy link
Contributor

rofl0r commented Feb 21, 2023

Are you talking about the foreach loop that installs the symlinks?

right.

if you think that there may be stuff in ../bin that's not in $PROGS (though, why should it?) you can maybe use a filter.

@Compyx
Copy link
Contributor Author

Compyx commented Feb 21, 2023

If we use a glob to install the symlinks, then if the user didn't run make before running make avail there will be no warning and no symlinks installed. The point of this PR is to warn the user about running make first, not to silently do nothing.

@rofl0r
Copy link
Contributor

rofl0r commented Feb 21, 2023

The point of this PR is to warn the user about running make first, not to silently do nothing.

in that case you can just add $(PROGS) to prerequisites of the avail target, so make avail will build the progs first if they dont exist.

avail: $(PROGS)

@Compyx
Copy link
Contributor Author

Compyx commented Feb 21, 2023

I know.

The disadvantage of that is that make avail requires root privileges, so when we add $(PROGS) as prerequisite for make avail, the binaries and all intermediate objects will be built as root, which means the user cannot run make or make clean again without first changing the ownership all of build objects back to theirs with sudo or su.

Which is why I didn't simply add $(PROGS) as dependency of avail.

@Compyx
Copy link
Contributor Author

Compyx commented Mar 2, 2023

Any movement on this? It's not exactly brain surgery I'm performing in this PR ;)

@mrdudz
Copy link
Contributor

mrdudz commented Mar 2, 2023

forgot :)

@mrdudz mrdudz merged commit 6fe649e into cc65:master Mar 2, 2023
@Compyx
Copy link
Contributor Author

Compyx commented Mar 2, 2023

Thanks! =)

@inexorabletash
Copy link
Contributor

inexorabletash commented Mar 3, 2023

FYI, broke my workflow:

https://github.com/a2stuff/build-install-ca65-action/blob/016c3208a7ebfaa7b6b98f7b090dc481bca12239/action.yml#L8

which only does:

sudo make -C /tmp/cc65 ca65 ld65 avail

ETA: I updated my workflow to "manually" link ca65 and ld65 targets. Just a heads-up that others may have taken similar shortcuts.

inexorabletash added a commit to a2stuff/build-install-ca65-action that referenced this pull request Mar 3, 2023
cc65's 'make avail' now fails unless all targets were built.
Create links explicitly for ca65/ld65 only.
inexorabletash added a commit to a2stuff/build-install-ca65-action that referenced this pull request Mar 3, 2023
cc65's 'make avail' now fails unless all targets were built.
Create links explicitly for ca65/ld65 only.
inexorabletash added a commit to a2stuff/build-install-ca65-action that referenced this pull request Mar 3, 2023
cc65's 'make avail' now fails unless all targets were built.
Create links explicitly for ca65/ld65 only.
@mrdudz
Copy link
Contributor

mrdudz commented Mar 3, 2023

FYI, broke my workflow:

https://github.com/a2stuff/build-install-ca65-action/blob/016c3208a7ebfaa7b6b98f7b090dc481bca12239/action.yml#L8

which only does:

sudo make -C /tmp/cc65 ca65 ld65 avail

ETA: I updated my workflow to "manually" link ca65 and ld65 targets. Just a heads-up that others may have taken similar shortcuts.

To be honest - i am thinking about removing this "avail" target altogether, because it is very non-unix and "install" is the right thing to do anyway.

@Compyx
Copy link
Contributor Author

Compyx commented Mar 3, 2023

I agree removing make avail and having a proper make install (including not having to manually provide PREFIX, but defaulting to /usr/local/) would definitely remove some of the unpleasant surprises of trying to install cc65. It would also simplify distributing cc65 since make avail depends on the source tree remaining present, which is a very peculiar requirement, to put it mildly.

@spiro-trikaliotis
Copy link

It would also simplify distributing cc65 since make avail depends on the source tree remaining present, which is a very peculiar requirement, to put it mildly.

But distributing cc65 is no problem (provide a PREFIX and use make install).
Please, if the install target is changed, please make sure to do it in a way that the packetizer can react on it in a timely manner. Having a default /usr/local is not a problem, but removing make targets or changing their behaviour might be.

@mrdudz
Copy link
Contributor

mrdudz commented Mar 4, 2023

Having a default /usr/local is not a problem, but removing make targets or changing their behaviour might be.

I don't think any packager uses "make avail" :)

@spiro-trikaliotis
Copy link

I don't think any packager uses "make avail" :)

Me neither. But: It was mentioned that removing make avail (and adding a default PREFIX) would simplify distribution. I don't think so: Providing a PREFIX should be very familiar for any packetizer, and avail is nothing one would do. So, I do not see how this would simplify distributions.

However, changing behaviour of install beyond providing a default PREFIX might be problematic, depending upon what it is exactly.

As I am packetizing cc65 myself, I only wanted to raise my voice to be careful when doing changes here.

@Compyx
Copy link
Contributor Author

Compyx commented Mar 5, 2023

Last time I used make install the assembler couldn't find the .mac files it required, they were installed in $PREFIX/share/cc65, but the assembler failed with a message about longjump. So I'd like to fix make install to the point you actually get a usable installation of cc65 that doesn't rely on the source tree being present.

Perhaps @mrdudz can move this into a discussion topic?

@mrdudz
Copy link
Contributor

mrdudz commented Mar 5, 2023

Perhaps @mrdudz can move this into a discussion topic?

Interestingly.... i cant do that here, for some reason (perhaps because it is merged?) - so better open a new discussion topic :)

acqn pushed a commit to acqn/cc65 that referenced this pull request Sep 18, 2023
make avail: check for presence of binaries before creating symbolic links
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants