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

Use classic names for bison generated files (in oc2/ only) #2925

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

georgthegreat
Copy link
Contributor

@georgthegreat georgthegreat commented May 15, 2024

This simplifies the command a bit and makes it easier to repeat the build routine in a different build system.

@CLAassistant
Copy link

CLAassistant commented May 15, 2024

CLA assistant check
All committers have signed the CLA.

@DennisHeimbigner
Copy link
Collaborator

The makeparser action in Makefile is normally only carried out when the .y file changes.
The renaming was done to avoid the builder having to have bison installed. Using the original *.tab.[ch]
names would, as I recall, have required this because:

  1. autoconf will forcibly delete files of the name .tab. (I assume it still does)
  2. the forcible deletion means that the person building the parser has to have bison installed.

You should verify that your changes do not require bison to be installed.

@georgthegreat
Copy link
Contributor Author

georgthegreat commented May 15, 2024

You should verify that your changes do not require bison to be installed.

They still don't require it, there are no changes in CMake / Makefiles logic.
I assume this is also confirmed by 106 successful checks from the CI.

One still can regenerate the files by running

cd netcdf-c/os2
make -f Makefile.am makeparser

(the latter actually suggests using automake, but simple make works as well for the purpose).

@DennisHeimbigner
Copy link
Collaborator

DennisHeimbigner commented May 16, 2024

Did some testing. The current situation is that dap.tab.[ch] is deleted automatically by make DISTCLEAN,
but not make CLEAN.
So, we have to decide: it is likely that a random user will execute DISTCLEAN; probably not.
So our assumption now becomes that if some user does DISTCLEAN, then he knows enough
to install bison and flex. This seems reasonable to me.
But there is a danger here. I could imagine some user doing DISTCLEAN just before pushing a PR
for some proposed change as a cleanup action. In that case, the *.tab.[ch] files will be missing
from the PR when they should be there.
Also, this change should be propagated to the other .y and .l files in e.g. ncgen.
Also, we need to document this bison requirement somewhere.

@georgthegreat
Copy link
Contributor Author

The current situation is that dap.tab.[ch] is deleted automatically by make DISTCLEAN,
but not make CLEAN.

I assumed that cmake is the modern way to build netcdf-c, isn't it?
Or are you supporting both?

CMake does not have any instructions for generating bison code on the fly, it just uses the files from the repo.

@dopplershift
Copy link
Member

But there is a danger here. I could imagine some user doing DISTCLEAN just before pushing a PR
for some proposed change as a cleanup action. In that case, the *.tab.[ch] files will be missing
from the PR when they should be there.

Just my $0.02 that this is why you don't check in any generated files into git. The source distribution should certainly contain them if you'd like to avoid users needing to build them, but that should be done as part of the release process.

@WardF
Copy link
Member

WardF commented May 16, 2024

There are a few irregularly-regenerated netCDF files; these are not generated with any regularity, and are checked in to version control because they always have been, from back in the pre-me subversion days. These are the files targeted by makeparser.

This simplifies the command a bit and makes it easier to repeat the build routine in a different build system.

Can you elaborate on this? I'm trying to suss out what the problem is that is being solved, at the cost of introducing additional complexity into our version control system. I'm not suggesting there isn't any value here, I'm just trying to weigh one against the other, especially as I'm trying to juggle some stuff to get the next release out already :).

@DennisHeimbigner Re: a user submitting a PR, it will be incumbent upon us to review any PR's to make sure a user hasn't pushed a PR that deletes necessary files. But hopefully we've been doing that so far, and if they were to push such a PR, presumably the CI would fail, as these files are not generated automatically.

@dopplershift These generated files are different than most typical use cases, insofar as they aren't generated with any regularity, but are instead generated due to the underlying combinatorics of data type conversions, testing, etc. So they are generated as opposed to written by hand (since the combinatorics follow a predictable pattern), but I think they are rightfully excepted from the 'don't keep generated files in version control' rule, especially since as it stands, there is no user dependency on bison unless they want build the makeparser target.

That said, it's possible the files are generated if they are missing in the first place, or if the target is manually invoked. Either way, since what we have works, and is transparent to the end user, and system-portable, I'm just curious about what is being addressed from a prioritization standpoint :). Thanks all!

@georgthegreat
Copy link
Contributor Author

Can you elaborate on this?

We are using custom build system (yatool, to be precise) which provides user with high-level primitives as it knows how to build many specific file formats (including native support for .y grammars).

The user just lists all the sources and everything else is done by the build system itself.
What I trying to achieve is to simplify netcdf-c description to the following:

SRCS(
    oc2/daplex.c
    oc2/dapparse.c
    oc2/dap.y
    oc2/oc.c
    ...
)

Passing oc2/dap.y will run bison under the hood, catch it output (known to the build system) and then compile this output.
While the build system is customisable, every customisation comes at a cost which I am trying to avoid to removing every customisation from netcdf-c itself.

I'm trying to suss out what the problem is that is being solved, at the cost of introducing additional complexity into our version control system.

I don't quite see any additional complexity here. Could you explain me where this PR adds to it?
From my point of view everything got (a bit) more simple than is was.

@georgthegreat
Copy link
Contributor Author

@WardF, lets proceed with the discussion / merging of this PR?

@DennisHeimbigner
Copy link
Collaborator

I just noticed that you made the change for the .y file in oc2.
You need to apply the same change to all the other .y files
in the netcdf-c code base. This would be to ensure that
no conflicts arise.

@georgthegreat
Copy link
Contributor Author

georgthegreat commented Oct 29, 2024

There is still no approval from the upstream if they are ready to apply this change at all.

I might proceed with this PR once it will get some degree of approval.

@DennisHeimbigner
Copy link
Collaborator

I did an experiment of applying this change to a second bison .y files (dce.y).
I wanted to make sure that there were no potential name conflicts, and there are none.
So, we can do this change (extended to all the .y files in the netcdf code base.
My caveat above about DISTCLEAN vs CLEAN still applies, however.

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.

5 participants