-
Notifications
You must be signed in to change notification settings - Fork 39
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
Use perl instead of sed #63
base: master
Are you sure you want to change the base?
Use perl instead of sed #63
Conversation
Have converted to draft as though it works on windows it doesn't on linux. The problem is that |
It might be possible to do something like ifeq ($(strip $(OS_CLASS)),WIN32)
QUOTE="
else
QUOTE='
endif
$(COMMON_DIR)/siteEnvVars.substitutions: $(EPICS_BASE)/configure/CONFIG_SITE_ENV
@echo Expanding siteEnvVars.substitutions from CONFIG_SITE_ENV....
@echo file iocEnvVar.template> $@
@echo {>> $@
@echo pattern>> $@
@echo { ENVNAME, ENVVAR, ENVTYPE }>> $@
@perl -n -e $(QUOTE)$$m = s/^EPICS_([A-Z_]*).*/{$${1}, EPICS_$${1}, epics}/; print $$_ if $$m;$(QUOTE) < $< >> $@
@echo }>> $@
though I am not sure this is much better... 😄 |
Well it makes it clear what we are trying to do at least, I am happy to go with that if you are - thanks |
Unless you think putting the |
Surely it would be simpler to use a stand-alone Perl script to do the whole job of generating the |
Oh no more perl scripts 😄 Anyhow, I think you are correct in this case that a standalone script is easier; this is a pretty simple parsing, but the solution is better. |
@@ -0,0 +1,9 @@ | |||
print("file iocEnvVar.template\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a shebang, usage, and copyright notice?
@echo "{ ENVNAME, ENVVAR, ENVTYPE }" >> $@ | ||
@sed -n 's/^EPICS_\([A-Z_]*\).*/{\1, EPICS_\1, epics}/p' $< >> $@ | ||
@echo "}" >> $@ | ||
@perl ../genSiteEnvVars.pl < $< > $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better to use $(PERL)
here instead of directly calling perl
This I leave up to you, but I think I would prefer a perl script that does not read from stdin but takes a file as an argument; looking at most of the perl scripts in use in various EPICS modules that seems to be more the more consistent standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on using $(PERL)
.
Most Base build rules that run generator scripts start with @$(RM) $@
to explicitly delete the target file first, so any errors which result in the script not actually regenerating the output file will trigger an error from GNUmake instead of it just continuing without recreating the file. I guess that's an enhancement request since the original rule didn't do that, but I recommend adding it anyway.
Switching from stdin to a filename argument might not need any changes to your Perl code at all, since using while (<>) { ... }
in Perl inline code automatically opens and parses the files listed in @ARGV
anyway (improve on that, Python!).
print("file iocEnvVar.template\n"); | ||
print("{\n"); | ||
print("pattern\n"); | ||
print("{ ENVNAME, ENVVAR, ENVTYPE }\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a here-doc could reduce the number of quotes and escaped newlines, and I recommend always quoting the argument to file
(again, not in the original). My personal preference for formatting substitution files would have fewer newlines:
print << __END__;
file "iocEnvVar.template" {
pattern { ENVNAME, ENVVAR, ENVTYPE }
__END__
Windows does not have
sed
so change to useperl
that is already needed for building EPICSSee #62